-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
@@ -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"] } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Let's wait.
There was a problem hiding this comment.
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"] } |
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, and
Features` is a comma-separated list of all enabled features. Other values self-explanatory.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- Features of dependencies are enabled if they're enabled in build-dependencies; breaks no_std libs rust-lang/cargo#5730
- Tracking issue for -Z features=host_dep rust-lang/cargo#7915
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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:
service
command is added to print detection of watchdogs and/or panic messagesservice
USB command, Booster is power cycled, or if the user manually resets interlocks using theInterlock Reset
buttonservice
is issued over USBThese changes are intended to provide us a minimal persistent state for debugging faulty Boosters in the field to aid in diagnosing future issues.
Testing
service
command reported it. Future calls toservice
indicated no watchdog was presentservice
command output. I verified it was cleared afterwards.