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

feat(reboot reason): reason published on mcu-util requested reboot #141

Merged
merged 4 commits into from
Jan 16, 2025

Conversation

sri9311
Copy link
Contributor

@sri9311 sri9311 commented Dec 30, 2024

worldcoin/orb-messages#42 needs to go in first, then dependency in west.yml will be updated in a follow-up commit

publish ShutdownScheduled msg w/JETSON_REQUESTED_REBOOT on mcu-util reboot

fixes ORBP-363

@sri9311 sri9311 requested a review from fouge as a code owner December 30, 2024 04:37
Copy link
Collaborator

@TheButlah TheButlah left a comment

Choose a reason for hiding this comment

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

approving, but consider getting @fouge to take a look

Copy link
Collaborator

@fouge fouge left a comment

Choose a reason for hiding this comment

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

you need to track the new head of https://github.com/worldcoin/orb-messages/ following the merge of worldcoin/orb-messages#42 in west.yml

Copy link
Collaborator

@fouge fouge left a comment

Choose a reason for hiding this comment

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

IMO we should rely on Memfault's reboot reason and depreciate that one which is far from complete unfortunately.

@TheButlah
Copy link
Collaborator

TheButlah commented Jan 7, 2025

hmm I don't know. I think I would prefer having our own reboot reason. Its our business logic so we should have control over our reboot reason, right? Rather than relying on what someone else considers to be the valid reboot reasons.

@fouge
Copy link
Collaborator

fouge commented Jan 8, 2025

@TheButlah
I added our own reboot reasons, on top of the system ones (hard faults etc).
see
https://github.com/worldcoin/orb-firmware/blob/main/main_board/config/memfault_reboot_reason_user_config.def

these are all the reboot reasons we currently track with Memfault.
Screenshot 2025-01-08 at 08 19 02

we could add a new JetsonRequestedReboot in handle_reboot_message and another new JetsonRequestedShutdown in handle_shutdown.

@fouge
Copy link
Collaborator

fouge commented Jan 14, 2025

@sri9311 anything i can do to help make progress on that task?

list of things to change:

  • report JetsonRequestedReboot from runner.c:handle_shutdown
  • add JetsonRequestedReboot and JetsonRequestedShutdown to Memfault reboot reasons
  • report JetsonRequestedReboot and JetsonRequestedShutdown events to Memfault

@sri9311
Copy link
Contributor Author

sri9311 commented Jan 14, 2025

@fouge I'll make these changes today, I was busy exploring JSON, forgot about this!

As discussed, proceeding only with JetsonRequestedReboot

publish ShutdownScheduled msg w/JETSON_REQUESTED_REBOOT on mcu-util reboot

Signed-off-by: Srikar Chintapalli <[email protected]>
bump orb-messages for change compatibility

Signed-off-by: Srikar Chintapalli <[email protected]>
Move CAN msg from shutdown to reboot handler and publish event on Memfault

Signed-off-by: Srikar Chintapalli <[email protected]>
@sri9311 sri9311 force-pushed the sri/mcu-reboot-request-ack branch from bf5652a to 47dfdeb Compare January 15, 2025 01:25
Copy link
Collaborator

@fouge fouge left a comment

Choose a reason for hiding this comment

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

I would make sure the reboot command has been successfully taken into account before sending the event and telling Memfault the reboot reason.

Comment on lines 318 to 330
// Send out shutdown scheduled CAN message
orb_mcu_main_ShutdownScheduled shutdown;
shutdown.shutdown_reason =
orb_mcu_main_ShutdownScheduled_ShutdownReason_JETSON_REQUESTED_REBOOT;
shutdown.has_ms_until_shutdown = true;
shutdown.ms_until_shutdown = delay * 1000;
publish_new(&shutdown, sizeof(shutdown),
orb_mcu_main_McuToJetson_shutdown_tag,
CONFIG_CAN_ADDRESS_DEFAULT_REMOTE);
#ifdef CONFIG_MEMFAULT
MEMFAULT_REBOOT_MARK_RESET_IMMINENT(
kMfltRebootReason_JetsonRequestedReboot);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should do all of this only if reboot() returns RET_SUCCESS below.

Otherwise, the reboot command failed and thus the event shouldn't be sent to the Jetson nor marked to Memfault.

send CAN message and Memfault event after successful reboot ret

Signed-off-by: Srikar Chintapalli <[email protected]>
@fouge fouge merged commit 3927fa2 into main Jan 16, 2025
11 checks passed
@fouge fouge deleted the sri/mcu-reboot-request-ack branch January 16, 2025 10:28
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.

3 participants