applications: serial_lte_modem: Fix asserts when pressing power pin #21081
Conversation
github-actions
bot
added
the
changelog-entry-required
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:more detailsGithub labels
List of changed files detected by CI (0)Outputs:ToolchainVersion: Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
trantanen
force-pushed
the
slm_powerpin_assert
branch
from
c3bf395 to
47c081e
Compare
| * so that it doesn't fire right away (which it does if enabled here). | ||
| */ | ||
| k_work_schedule(&work, K_MSEC(1)); | ||
| configure_power_pin_interrupt(power_pin_callback_poweroff, GPIO_INT_EDGE_RISING); |
There was a problem hiding this comment.
@tomi-font Do you remember what happened if GPIO was configured here? The comment says it would have fired immediately but I haven't experienced issues with this.
There was a problem hiding this comment.
Would this have been because we were level triggered at some point? Cannot recall exactly.
| SHELL_CMD_REGISTER(slm, NULL, "Send AT commands to SLM device", modem_slm_shell); | ||
|
|
||
| SHELL_STATIC_SUBCMD_SET_CREATE( | ||
| sub_slmsh, |
There was a problem hiding this comment.
I've made slmsh as the main command for shell commands that are executed in the SLM shell device. Better naming for "SLM shell device" or for the command name are welcome.
|
You can find the documentation preview for this PR here. |
trantanen
force-pushed
the
slm_powerpin_assert
branch
from
47c081e to
466eb13
Compare
| * so that it doesn't fire right away (which it does if enabled here). | ||
| */ | ||
| k_work_schedule(&work, K_MSEC(1)); | ||
| configure_power_pin_interrupt(power_pin_callback_poweroff, GPIO_INT_EDGE_RISING); |
There was a problem hiding this comment.
Would this have been because we were level triggered at some point? Cannot recall exactly.
| @@ -276,12 +273,21 @@ static void power_pin_callback_wakeup(const struct device *dev, | |||
| return; | |||
| } | |||
|
|
|||
| atomic_set(&callback_running, false); | |||
There was a problem hiding this comment.
This needs to be in power_pin_callback_wakeup. Level triggered GPIO can cause multiple calls to power_pin_callback_wakeup. I am uncertain whether removing the callback as the very first thing would prevent this.
Also, in older version, the callback is only removed when we have brought the UART up. Now if the power_pin_callback_wakeup_work_fn somehow fails, the only way to bring the UART up is to reset the SLM from the external MCU.
There was a problem hiding this comment.
Both of these topics are something I've been thinking a lot when doing the change.
This needs to be in power_pin_callback_wakeup. Level triggered GPIO can cause multiple calls to power_pin_callback_wakeup. I am uncertain whether removing the callback as the very first thing would prevent this.
I haven't seen problems in form of multiple calls to power_pin_callback_wakeup() function when pressing power pin. I could move the following block from power_pin_callback_wakeup_work_fn() to power_pin_callback_wakeup() if needed:
/* Prevent level triggered interrupt running this multiple times. */
if (!atomic_cas(&callback_running, false, true)) {
return;
}
But I would say that then atomic_set(&callback_running, false); should still be here as it is.
However, if we determine that gpio_remove_callback function call handles this properly then the whole callback_running could be removed.
Also, in older version, the callback is only removed when we have brought the UART up. Now if the power_pin_callback_wakeup_work_fn somehow fails, the only way to bring the UART up is to reset the SLM from the external MCU.
I had mistakenly thought that since we set configure_power_pin_interrupt(power_pin_callback_enable_poweroff, GPIO_INT_EDGE_RISING); in power_pin_callback_wakeup(), this would be handled into some extent but that will then power off the SLM device instead of return to "normal mode" from idle. I'm adding setting of these GPIOs and callbacks into power_pin_callback_wakeup_work_fn() and also into the error branch.
Asserts were enabled with PR nrfconnect#20040. This has caused an assert to occur when pressing power pin, because GPIO callback (power_pin_callback_wakeup) called slm_at_host_power_on, which called k_sleep(), which has an assert that it's not called from interrupt context. Added most of the wake up operation into a worker. Jira: LRCS-70 Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
Add command for toggling power pin. Also adding main command for operations done in SLM shell device. Toggling is done as follows: "slmsh powerpin" Jira: LRCS-84 Signed-off-by: Tommi Rantanen <tommi.rantanen@nordicsemi.no>
trantanen
force-pushed
the
slm_powerpin_assert
branch
from
466eb13 to
e1fb005
Compare
|
Asserts were enabled with PR #20040.
This has caused an assert to occur when pressing power pin, because GPIO callback (
power_pin_callback_wakeup()) calledslm_at_host_power_on(), which calledk_sleep(), which has an assert that it's not called from interrupt context. Added most of the wake up operation into a worker.Also added a command to
modem_slmlibrary for toggling power pin. In addition, adding main command for operations done in SLM shell device. Toggling power pin is done as follows:"slmsh powerpin"
I have some follow-up PRs for SLM shell so I'll also update its documentation then regarding the new command. I'd like to get these changes in whenever they are technically OK so that test engineers can proceed testing the power pin.