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

Adding support for reporting service information #131

Merged
merged 6 commits into from
Jan 20, 2021

Conversation

ryan-summers
Copy link
Member

@ryan-summers ryan-summers commented Jan 18, 2021

This fixes #93

As a debug tool, I've added in panic-persist and refactored how the watchdog operates.

With the new behavior, the following functionality is added:

  • A new USB service command is added to print detection of watchdogs and/or panic messages
  • The watchdog detection is only reset by the user - all channels will remain disabled unless the user clears the watchdog detection via the service USB command, Booster is power cycled, or if the user manually resets interlocks using the Interlock Reset button
  • Service information will persist until Booster is reset or until the service is issued over USB

These changes are intended to provide us a minimal persistent state for debugging faulty Boosters in the field to aid in diagnosing future issues.

Testing

  • I have induced a watchdog reset and verified the service command reported it. Future calls to service indicated no watchdog was present
  • I induced a panic and verified the message was shown in the service command output. I verified it was cleared afterwards.

@@ -25,6 +25,7 @@ shared-bus = { version = "0.2.0-alpha.1", features = ["cortex-m"] }
usb-device = "0.2.6"
postcard = "0.5.1"
crc-any = { version = "2.3.5", default-features = false }
panic-persist = { git = "https://github.com/jamesmunns/panic-persist", branch = "master", features = ["custom-panic-handler"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

James Munns said he would push the master branch to a new release later today, so we should wait and update when that gets pushed.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Let's wait.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deferring to #136

@@ -25,6 +25,7 @@ shared-bus = { version = "0.2.0-alpha.1", features = ["cortex-m"] }
usb-device = "0.2.6"
postcard = "0.5.1"
crc-any = { version = "2.3.5", default-features = false }
panic-persist = { git = "https://github.com/jamesmunns/panic-persist", branch = "master", features = ["custom-panic-handler"] }
Copy link
Member

Choose a reason for hiding this comment

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

Yes. Let's wait.

@@ -152,6 +156,25 @@ impl SerialTerminal {
);
}

Request::ServiceInfo => {
Copy link
Member

Choose a reason for hiding this comment

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

Also print version, release/debug, features and commit hash (through build.rs I guess). That makes the dump much more useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Service command has now become much more comprehensive:

> service
Version             : v0.1.0-50-gf59fce4
Git revision        : f59fce4f44f3790adf26d6be25a5917cb32793af-dirty
Features            : UNSTABLE
Panic Info          : None
Watchdog Detected   : false

> 

Version indicates the output of git describe --tags, Git revisionappends the-dirtyflag to indicate if untracked changes are present, andFeatures` is a comma-separated list of all enabled features. Other values self-explanatory.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just use built (dev-dependency) for that. Also makes the git parsing and command execution somewhat more robust.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jordens I was poking around with built and discovered an issue with cargo that prevents us from using built along with stable cargo - built enables std-features in serde, which is then (incorrectly) propagated to our application, which results in the app build failing if we use built in the [build-dependencies] section.

Necessary cargo changes were merged recently, but we'd have to move to nightly cargo.

We can move over to nightly cargo, but that would not be super optimal. We could alternatively leave this minimal implementation and spawn an issue to update to built when cargo stabilizes the fix.

Copy link
Member

Choose a reason for hiding this comment

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

Ah. I wasn't aware that crate features were also unified between dependencies and build-dependencies. That sounds bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bug that just recently got fixed, but is not quite yet on stable cargo

@ryan-summers ryan-summers requested a review from jordens January 20, 2021 11:19
@jordens jordens mentioned this pull request Jan 20, 2021
@ryan-summers ryan-summers merged commit b50bfa9 into develop Jan 20, 2021
@ryan-summers ryan-summers deleted the feature/service-status branch January 20, 2021 13:49
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.

crash/reset post mortem
2 participants