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.

Sign up for GitHub

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

Jump to bottom

[Silabs] Adds fix for BTN low power mode for SiWx917 SoC and power manager #35808

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
+58 −34
Open

[Silabs] Adds fix for BTN low power mode for SiWx917 SoC and power manager #35808

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fa56805
Decouple UART power requirement handler
rosahay-silabs Sep 19, 2024
56c863d
Adds refactored BTN handler based on power manager
rosahay-silabs Sep 26, 2024
1484d09
Fixed incorrect API
rosahay-silabs Sep 27, 2024
6f3aff4
Adds changes for interrupt
rosahay-silabs Sep 30, 2024
9cd54a5
Replace use of RSI_xx APIs with sl_si91x_xx APIs
rosahay-silabs Sep 30, 2024
01b44d9
Refactor button event handling and UART power requirement handler
rosahay-silabs Oct 3, 2024
5be0a39
Updated Support SDK with BTN0 wakeup source changes
rosahay-silabs Oct 3, 2024
ccd1897
Merge branch 'master' into bugfix/siwx917-sleep-btn0-fix
rosahay-silabs Oct 21, 2024
a91f0af
Merge branch 'master' into bugfix/siwx917-sleep-btn0-fix
rosahay-silabs Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion examples/platform/silabs/MatterConfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,7 @@ CHIP_ERROR SilabsMatterConfig::InitWiFi(void)
extern "C" void vApplicationIdleHook(void)
{
#if (SLI_SI91X_MCU_INTERFACE && CHIP_CONFIG_ENABLE_ICD_SERVER)
sl_si91x_invoke_btn_press_event();
sl_si91x_btn_event_handler();
sl_si91x_uart_power_requirement_handler();
rosahay-silabs marked this conversation as resolved.
Show resolved Hide resolved
#endif
}
56 changes: 34 additions & 22 deletions examples/platform/silabs/SiWx917/SiWx917/sl_wifi_if.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,12 @@ extern "C" {

#if CHIP_CONFIG_ENABLE_ICD_SERVER && SLI_SI91X_MCU_INTERFACE
#include "rsi_rom_power_save.h"
#include "sl_gpio_board.h"
#include "sl_si91x_button.h"
#include "sl_si91x_button_pin_config.h"
#include "sl_si91x_driver_gpio.h"
#include "sl_si91x_power_manager.h"

namespace {
// TODO: should be removed once we are getting the press interrupt for button 0 with sleep
#define BUTTON_PRESSED 1
bool btn0_pressed = false;

#ifdef ENABLE_CHIP_SHELL
bool ps_requirement_added = false;
#endif // ENABLE_CHIP_SHELL
Expand Down Expand Up @@ -262,23 +260,36 @@ sl_status_t join_callback_handler(sl_wifi_event_t event, char * result, uint32_t

#if CHIP_CONFIG_ENABLE_ICD_SERVER
#if SLI_SI91X_MCU_INTERFACE
// Required to invoke button press event during sleep as falling edge is not detected
void sl_si91x_invoke_btn_press_event(void)
// This function is called when the button is pressed when in sleep then the idleTask is handling the button press event
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you adapt these comments to the appropriate format?
This comment applies to all functions.

/**
 * @brief      invoked when enhanced connection complete event is received
 * @param[out] resp_conn, connected remote device information
 * @return     none.
 */

Can you add to the comment that the signature is important since it is a weak function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, resolved but not addressed.

void gpio_uulp_pin_interrupt_callback(uint32_t pin_intr)
{
// TODO: should be removed once we are getting the press interrupt for button 0 with sleep
if (!RSI_NPSSGPIO_GetPin(SL_BUTTON_BTN0_PIN) && !btn0_pressed)
// UULP_GPIO_2 is used to detect the button 0 press
VerifyOrReturn(pin_intr == RTE_UULP_GPIO_2_PIN, ChipLogError(DeviceLayer, "invalid pin interrupt: %ld", pin_intr));
sl_status_t status = SL_STATUS_OK;
uint8_t pin_intr_status = sl_si91x_gpio_get_uulp_npss_pin(pin_intr);
if (pin_intr_status == LOW)
{
sl_button_on_change(SL_BUTTON_BTN0_NUMBER, BUTTON_PRESSED);
btn0_pressed = true;
}
if (RSI_NPSSGPIO_GetPin(SL_BUTTON_BTN0_PIN))
{
btn0_pressed = false;
// BTN_0 is pressed
// NOTE: the GPIO is masked since the interrupt is invoked before scheduler is started, thus this is required to hand over
// control to scheduler, the PIN is unmasked in the power manager flow before going to sleep
status = sl_si91x_gpio_driver_mask_uulp_npss_interrupt(BIT(pin_intr));
VerifyOrReturn(status == SL_STATUS_OK, ChipLogError(DeviceLayer, "failed to mask interrupt: %ld", status));
}
}
// Required to invoke button press event during sleep as falling edge is not detected
// NOTE: flow is GPIO wakeup due to BTN0 press -> check button state in idle task
// required as the GPIO interrupt is not detected during sleep for BUTTON RELEASED
void sl_si91x_btn_event_handler(void)
rosahay-silabs marked this conversation as resolved.
Show resolved Hide resolved
{
sl_button_on_change(SL_BUTTON_BTN0_NUMBER,
(sl_si91x_gpio_get_uulp_npss_pin(SL_BUTTON_BTN0_PIN) == LOW) ? BUTTON_PRESSED : BUTTON_RELEASED);
}

void sl_si91x_uart_power_requirement_handler(void)
{
#ifdef ENABLE_CHIP_SHELL
// Checking the UULP PIN 1 status to reinit the UART and not allow the device to go to sleep
if (RSI_NPSSGPIO_GetPin(RTE_UULP_GPIO_1_PIN))
if (sl_si91x_gpio_get_uulp_npss_pin(RTE_UULP_GPIO_1_PIN))
{
if (!ps_requirement_added)
{
Expand Down Expand Up @@ -407,12 +418,13 @@ static sl_status_t wfx_rsi_init(void)
#ifdef ENABLE_CHIP_SHELL
// While using the matter shell with the ICD server, the GPIO 1 is used to check the UULP PIN 1 status
// since UART doesn't act as a wakeup source in the UULP mode
/*Configuring the NPS GPIO 1*/
RSI_NPSSGPIO_SetPinMux(RTE_UULP_GPIO_1_PIN, 0);
/*Configure the NPSS GPIO direction to input */
RSI_NPSSGPIO_SetDir(RTE_UULP_GPIO_1_PIN, 1);
/*Enable the REN*/
RSI_NPSSGPIO_InputBufferEn(RTE_UULP_GPIO_1_PIN, 1);
/* Configuring the NPS GPIO 1 */
sl_si91x_gpio_set_uulp_npss_pin_mux(RTE_UULP_GPIO_1_PIN, sl_si91x_uulp_npss_mode_t.NPSS_GPIO_PIN_MUX_MODE0);
/* Configure the NPSS GPIO direction to input */
sl_si91x_gpio_set_uulp_npss_direction(RTE_UULP_GPIO_1_PIN, sl_si91x_gpio_direction_t.GPIO_INPUT);
/* Enable the REN */
sl_si91x_gpio_select_uulp_npss_receiver(RTE_UULP_GPIO_0_PIN, sl_si91x_gpio_receiver_t.GPIO_RECEIVER_EN)

#endif // ENABLE_CHIP_SHELL
#endif /* CHIP_CONFIG_ENABLE_ICD_SERVER */
#endif /* SLI_SI91X_MCU_INTERFACE */
Expand Down
5 changes: 4 additions & 1 deletion examples/platform/silabs/wfx_rsi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@ int32_t wfx_rsi_disconnect();
int32_t wfx_wifi_rsi_init(void);
#if CHIP_CONFIG_ENABLE_ICD_SERVER
#if SLI_SI91X_MCU_INTERFACE
void sl_si91x_invoke_btn_press_event();
void sl_si91x_btn_event_handler();
void sl_si91x_uart_power_requirement_handler();
// this is callback from the Wiseconnect SDK
void gpio_uulp_pin_interrupt_callback(uint32_t pin_intr);
Comment on lines +109 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

These functions seem to be specific to the 917 but are in a header file that is common to all wi-fi platforms.
Can we start moving these function out of the "global" header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mkardous-silabs , do you suggest keeping something like sl_si91x_hal.h instead? Since we do not have anything like that now on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Call it SiWxPlatformInterface.h for now and we can see later its finale name.
Put these functions inside a SiWxPlatform namespace.

#endif // SLI_SI91X_MCU_INTERFACE
#if SLI_SI917
int32_t wfx_rsi_power_save(rsi_power_save_profile_mode_t sl_si91x_ble_state, sl_si91x_performance_profile_t sl_si91x_wifi_state);
Expand Down
21 changes: 13 additions & 8 deletions src/platform/silabs/platformAbstraction/WiseMcuSpam.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ namespace Silabs {
namespace {
uint8_t sButtonStates[SL_SI91x_BUTTON_COUNT] = { 0 };
#if CHIP_CONFIG_ENABLE_ICD_SERVER
bool btn0_pressed = false;
bool sl_btn0_pressed = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure i see the benefit of renaming this variable with sl_ when it is defined in a silabs specific file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is still being used in the global anonymous namespace right? and has the possibility to collide in future right?

Copy link
Contributor

Choose a reason for hiding this comment

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

global anonymous namespace does not exist. There are only three types or namespaces : global, named and anounymous. When something is in an anonymous namespace, it limits the scope of the element to the file were it was declared.

In this case, the only way for a collission, would be add a second variable btn0Pressed in the .cpp.

#endif /* SL_ICD_ENABLED */
} // namespace

Expand Down Expand Up @@ -143,21 +143,26 @@ void sl_button_on_change(uint8_t btn, uint8_t btnAction)
// Currently the btn0 is pull-up resistor due to which is sends a release event on every wakeup
if (btn == SL_BUTTON_BTN0_NUMBER)
{
if (btnAction == BUTTON_PRESSED)
if ((btnAction == BUTTON_PRESSED) && (sl_btn0_pressed == false))
{
btn0_pressed = true;
sl_btn0_pressed = true;
rosahay-silabs marked this conversation as resolved.
Show resolved Hide resolved
}
else if ((btnAction == BUTTON_RELEASED) && (btn0_pressed == false))
else if ((btnAction == BUTTON_RELEASED) && (sl_btn0_pressed == true))
{
// if the btn was not pressed and only a release event came, ignore it
sl_btn0_pressed = false;
}
else if ((btnAction == BUTTON_PRESSED) && (sl_btn0_pressed == true))
{
// if the btn was already pressed and another press event came, ignore it
return;
}
else if ((btnAction == BUTTON_RELEASED) && (btn0_pressed == true))
else if ((btnAction == BUTTON_RELEASED) && (sl_btn0_pressed == false))
{
btn0_pressed = false;
// if the btn was not pressed and only a release event came, ignore it
return;
Comment on lines +154 to +162
Copy link
Contributor

Choose a reason for hiding this comment

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

These conditions should be added at the start of the function with the VerifyOrReturn structure.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rosahay-silabs This marked resolved but was not addressed nor commented.

}
}
#endif /* SL_ICD_ENABLED */
#endif // SL_ICD_ENABLED
if (Silabs::GetPlatform().mButtonCallback == nullptr)
{
return;
Expand Down
5 changes: 4 additions & 1 deletion third_party/silabs/SiWx917_sdk.gni
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,12 @@ template("siwx917_sdk") {
"SL_ACTIVE_MODE_DURATION_MS=${sl_active_mode_duration_ms}",
"SL_IDLE_MODE_DURATION_S=${sl_idle_mode_duration_s}",
"SL_ICD_SUPPORTED_CLIENTS_PER_FABRIC=${sl_icd_supported_clients_per_fabric}",
"SL_SI91X_NPSS_GPIO_BTN_HANDLER=1",
"SL_SI91X_POWER_MANAGER_UC_AVAILABLE=1",
"SL_SI91X_TICKLESS_MODE=1",

# Enable Wakeup source for ICD
"SL_ENABLE_GPIO_WAKEUP_SOURCE=1",
"ENABLE_NPSS_GPIO_2=1",
]
}

Expand Down
2 changes: 1 addition & 1 deletion third_party/silabs/matter_support
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

There a multiple PRs updating the matter_support submodule, we need to make sure that we don't cause ourselves a incorrect hash based on the merged order.

Submodule matter_support updated 3 files
+6 −1 matter/si91x/siwx917/BRD4338A/autogen/sl_si91x_power_manager_wakeup_handler.c
+3 −4 matter/si91x/siwx917/BRD4338A/autogen/sl_si91x_power_manager_wakeup_handler.h
+2 −2 matter/si91x/siwx917/BRD4338A/config/sl_si91x_power_manager_wakeup_source_config.h
Loading