Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

samples: matter: Unification of events, buttons and LEDs #10

Closed
wants to merge 4 commits into from

Conversation

ArekBalysNordic
Copy link
Owner

This commit unifies events, buttons and LEDs for all Matter samples.

Changelog in this commit:

  • Separated Events to the system-specific and application-specific.
  • Created EventManager module which is responsible for posting and dispatching events.
  • Created the common board configuration file (board_config.h) that contains all board definitions, which can be overwritten in the app_config.h file in the sample directory.
  • Created the common board_const.h file that contains all constant definitions of timeouts and LED blinking intervals. Those definitions are the same for all samples and boards.
  • Created the BoardInterface module in board_interface.c/.h files. The BoardInterface module:
    • Initializes LEDs and buttons on the DK, taking into account the number of LEDs and buttons on the DK.
    • Manages LEDs and buttons operations.
    • Manages Matter Identify service.
    • Manages to start Bluetooth LE Advertising using a button.
    • Exposes a button callback which can be used in the Application code. Users have 3 or 1 available buttons and LEDs depending on the availability on the DK.
    • Manages the factory reset service.
  • Aligned Matter Lock sample to all changes.
    • Aligned Lock operations after clicking the button.
    • Aligned WiFi-Thread switching.
  • Limitations:
    • Events cannot be called directly from the Timer timeout event context.

This commit unifies events, buttons and LEDs for all Matter
samples.

Changelog in this commit:

- Separated Events to the system-specific and application-specific.
- Created EventManager module which is responsible for posting and
dispatching events.
- Created the common board configuration file (board_config.h)
that contains all board definitions, which can be overwritten in
the app_config.h file in the sample directory.
- Created the common board_const.h file that contains all constant
definitions of timeouts and LED blinking intervals.
Those definitions are the same for all samples and boards.
- Created the BoardInterface module in board_interface.c/.h files.
The BoardInterface module:
	- Initializes LEDs and buttons on the DK, taking
into account the number of LEDs and buttons on the DK.
	- Manages LEDs and buttons operations.
	- Manages Matter Identify service.
	- Manages to start Bluetooth LE Advertising using a button.
	- Exposes a button callback which can be used in the
Application code. Users have 3 or 1 available buttons and LEDs
depending on the availability on the DK.
	- Manages the factory reset service.
- Aligned Matter Lock sample to all changes.
	- Aligned Lock operations after clicking the button.
	- Aligned WiFi-Thread switching.
- Limitations:
	- Events cannot be called directly from the Timer timeout
event context.

Signed-off-by: Arkadiusz Balys <arkadiusz.balys@nordicsemi.no>
samples/matter/common/src/board_config.h Outdated Show resolved Hide resolved
samples/matter/common/src/board_interface.cpp Outdated Show resolved Hide resolved
samples/matter/common/src/board_interface.cpp Outdated Show resolved Hide resolved
samples/matter/common/src/board_interface.cpp Outdated Show resolved Hide resolved
samples/matter/common/src/board_interface.cpp Outdated Show resolved Hide resolved
samples/matter/common/src/event_manager.cpp Outdated Show resolved Hide resolved
samples/matter/common/src/event_manager.cpp Outdated Show resolved Hide resolved
samples/matter/common/src/event_manager.h Outdated Show resolved Hide resolved
samples/matter/common/src/board_interface.h Outdated Show resolved Hide resolved
void UpdateDeviceState(DeviceState state);

private:
BoardInterface() {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BoardInterface() {}
BoardInterface() = default;

or no explicit constructor at all I guess.


private:
BoardInterface() {}
friend BoardInterface &GetBoardInterface();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need a static instance? It is only used internally in the class implementation, right? Is it only because some methods need to be static?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because we can use that static instance outside the class implementation, for example in AppTask to get LEDs,

samples/matter/common/src/board_interface.h Outdated Show resolved Hide resolved
samples/matter/common/src/board_interface.h Outdated Show resolved Hide resolved
samples/matter/common/src/event_manager.h Outdated Show resolved Hide resolved
samples/matter/common/src/event_manager.h Outdated Show resolved Hide resolved
samples/matter/common/src/system_event.h Outdated Show resolved Hide resolved
samples/matter/lock/src/app_event.h Outdated Show resolved Hide resolved
/**
* @brief Construct a new App Event object from context
*
* User must ensure that context exists, is defined and has the same size as the AppEvent class,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it that in the current solution, the context must be exactly of AppEvent type?

@Damian-Nordic
Copy link
Collaborator

Damian-Nordic commented Dec 6, 2023

I wonder... is using the current Event structure that useful at all? It basically carries the handler function pointer and some extra context, but the event type is what makes the solution not generic enough and requires unsafe void* casts. What if we did something completely different like:

using Task = std::function<void()>;

class AppTaskExecutor
{
public:
    void PostTask(const Task& task) { /* add task to the queue */ }
    void DispatchNextTask() { auto task = /* dequeue task */; task(); }
    
    static AppTaskExecutor& Instance();
};

class CommonButtonModule
{
    void SomeDkButtonsAndLedCallback()
    {
        if (/* button 1 pressed */) {
            AppTaskExecutor::Instance().PostTask([this] { FactoryReset(); })
        }
    }

    void FactoryReset()
    {
        LOG_INF("Triggerring factory reset");
        Server::Instance().ScheduleFactoryReset();
    }
};

We could use Matter's LambdaBridge instead of std::function to keep it more lightweight. But the idea is to use lambdas instead of semi-polymorphic event type and unsafe casting to void*.

I haven't thought it through very much so just throwing an idea for now, but we can have a meeting and brainstorm on that if you believe it could simplify things.

source = DeviceButtons::kAppButton;
action = (APPLICATION_BUTTON_MASK & buttonState) ? ButtonActions::kButtonPressed :
ButtonActions::kButtonReleased;
isAppButtonEvent = true;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that DK buttons and LEDs module supports registering multiple handlers via https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/libraries/others/dk_buttons_and_leds.html#c.dk_button_handler_add, so an application could just register an extra handler to take care of app-specific buttons.

@ArekBalysNordic
Copy link
Owner Author

I wonder... is using the current Event structure that useful at all? It basically carries the handler function pointer and some extra context, but the event type is what makes the solution not generic enough and requires unsafe void* casts. What if we did something completely different like:

using Task = std::function<void()>;

class AppTaskExecutor
{
public:
    void PostTask(const Task& task) { /* add task to the queue */ }
    void DispatchNextTask() { auto task = /* dequeue task */; task(); }
    
    static AppTaskExecutor& Instance();
};

class CommonButtonModule
{
    void SomeDkButtonsAndLedCallback()
    {
        if (/* button 1 pressed */) {
            AppTaskExecutor::Instance().PostTask([this] { FactoryReset(); })
        }
    }

    void FactoryReset()
    {
        LOG_INF("Triggerring factory reset");
        Server::Instance().ScheduleFactoryReset();
    }
};

We could use Matter's LambdaBridge instead of std::function to keep it more lightweight. But the idea is to use lambdas instead of semi-polymorphic event type and unsafe casting to void*.

I haven't thought it through very much so just throwing an idea for now, but we can have a meeting and brainstorm on that if you believe it could simplify things.

It looks great, and is so simple... :) I will try to add it to the code and then we can discuss it as you mentioned.

@ArekBalysNordic
Copy link
Owner Author

I've refactored the event management system as Damian suggested and now it looks much better! Please take a look @kkasperczyk-no @markaj-nordic @Damian-Nordic. It requires fewer lines of code and is much less unsafe now. Thanks @Damian-Nordic!

The next step is to consider LambdaBridge, but @markaj-nordic do we want the Matter core code there? What do you think?

@ArekBalysNordic
Copy link
Owner Author

ArekBalysNordic commented Dec 6, 2023

I've addressed part of the @markaj-nordic comments, but not all yet.

ArekBalysNordic pushed a commit that referenced this pull request May 10, 2024
-This manifest update and relevant commits in sdk-nrf is made to add
 support for nRF54H20 devices without a local build of the core
-This includes taking in sdk-mbedtls pull request #15 for better
 client support in PK and MD wrapper APIs
-This includes taking in sdk-oberon-psa-crypto pull request #10 for
 better client support as well as fixups for psa_util used for
 TLS/DTLS and X.509 testing

Ref: NCSDK-15632

Signed-off-by: Frank Audun Kvamtrø <frank.kvamtro@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants