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

[SL-UP] Moving the high-performance request command out of the timer handler #215

Open
wants to merge 5 commits into
base: release_2.5-1.4
Choose a base branch
from

Conversation

senthilku
Copy link
Contributor

Description:
Issuing the power-save enable commands from the timer handler was causing a timeout issue, and the high-performance request before joining the network was failing.

Fix:
Moved the high-performance request command out of the timer handler.

Testing:
Tested commissioning and power cycling locally with the 917 SoC.

@senthilku senthilku requested a review from a team as a code owner January 20, 2025 06:48
Copy link
Contributor Author

@senthilku senthilku left a comment

Choose a reason for hiding this comment

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

Observing some issue when testing with latest commit.

@mkardous-silabs
Copy link
Contributor

mkardous-silabs commented Jan 22, 2025

The PR changes the behavior of ICD exampe application but testing section does not define any tests that verify that sleep behavior is not significantly impacted.

  • Does selective listening still work?
  • Does the app go to sleep when it is expected to do so? what were the test scenarios?
  • This change seem to conflict / duplicate the change chirag made, can we validate that both are requried?
  • What happens if we fail to join the Wi-Fi network, do we stay in high performance indefinitely?

Copy link
Contributor

@mkardous-silabs mkardous-silabs left a comment

Choose a reason for hiding this comment

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

Requesting change until the validated scenarios are clarified and tested.

@senthilku
Copy link
Contributor Author

The PR changes the behavior of ICD exampe application but testing section does not define any tests that verify that sleep behavior is not significantly impacted.

  • Does selective listening still work?
    Tested selective listening feature and it is working fine.
  • Does the app go to sleep when it is expected to do so? what were the test scenarios?
    The below scenarios are tested with lock application
  - Device is going to sleep before commissioning
  - Device is in Active stathe during commissioning
  - Device is going to sleep (DTIM based ) after commissioning complete
  - Device is going to sleep after power cycle
  - Device is switching between Active and sleep mode during retry connection (active before connection and sleep after connection failure)
  - Device is going to sleep after connection successful
  - Device is going to selective LI with subscription enabled
  • This change seem to conflict / duplicate the change chirag made, can we validate that both are requried?
    yes, This change is required, Need to remove High performance request that might have been added during the connect/retry process, or else device will be always active after successful connection
  • What happens if we fail to join the Wi-Fi network, do we stay in high performance indefinitely?
    Device is switching between Active and sleep mode during retry connection (active before connection and sleep after connection failure)

@mkardous-silabs
Copy link
Contributor

If i am not mistaken, this prevents the device from going to sleep while connecting to the Wi-Fi network. In practice, this shouldn't be the case. Can we move these high performance calls else where?

When we remove chirag's changes, this change will still impact power consumption and code execution.

@senthilku
Copy link
Contributor Author

If i am not mistaken, this prevents the device from going to sleep while connecting to the Wi-Fi network. In practice, this shouldn't be the case. Can we move these high performance calls else where?

When we remove chirag's changes, this change will still impact power consumption and code execution.

It is expected to remain in a high-performance state while connecting to the Wi-Fi network.
In the existing code, this was achieved by calling the high-performance function from the timer handler during a retry connection. In the PR, the high-performance call was moved from the timer event handler because it is a blocking call, causing the high-performance request to fail (timeout) every time

@mkardous-silabs
Copy link
Contributor

I understand why the call cannot be in the timer callback; it shouldn't have been put there from the start.
I'm not sure i understand whyIt is expected to remain in a high-performance state while connecting to the Wi-Fi network.

Outisde of the Wi-Fi issue which Chirag opened a PR for, it is possible for the device to connect to Wi-Fi network without being in high-performance.

The goal of having the high performance req in the timer callback, even if it is wrong, was to take the device out of the sleep with no Wi-Fi network state. In other words, it aimed to solve a specific use-case. I don't think it it is necessary for the board to be in high performance. The change was simply "copied" from the previous logic to enable the board to sleep from the start.

The change you are proposing, changes the device connect behavior outside of just fixing the bug. I do not believe this is the correct way to fix this issue.

@senthilku
Copy link
Contributor Author

I understand why the call cannot be in the timer callback; it shouldn't have been put there from the start. I'm not sure i understand whyIt is expected to remain in a high-performance state while connecting to the Wi-Fi network.
Wi-Fi team recomends to keep the High Performance mode before connecting to AP - https://jira.silabs.com/browse/SI91X-15845
Outisde of the Wi-Fi issue which Chirag opened a PR for, it is possible for the device to connect to Wi-Fi network without being in high-performance.

The goal of having the high performance req in the timer callback, even if it is wrong, was to take the device out of the sleep with no Wi-Fi network state. In other words, it aimed to solve a specific use-case. I don't think it it is necessary for the board to be in high performance. The change was simply "copied" from the previous logic to enable the board to sleep from the start.

The change you are proposing, changes the device connect behavior outside of just fixing the bug. I do not believe this is the correct way to fix this issue.

  • Wi-Fi team recommends to keep the High-Performance mode before connecting to AP - https://jira.silabs.com/browse/SI91X-15845
  • Switching between power modes in LMAC results in interoperability issues. Therefore, it is recommended to switch back to High Performance mode before connecting to the AP
  • So, I have moved the High-Performance request to occur before connecting to avoid connection issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants