feat(p987): report why frames fail to decode - #230
Conversation
A frame that produces no signals never reaches the live path — it is stored with a status in its metadata and dropped. That is the right behaviour, but it was entirely silent, so "frames are arriving and nothing shows up live" could only be diagnosed by querying ClickHouse. Count every decode outcome and report it. Each distinct (bus, can id, reason) logs once when first seen, so a new unknown id is visible immediately; repeats roll up every 30s into one line carrying the decoded and undecodable totals plus the worst offenders. Frames arrive at a few hundred per second, so a line per failure would bury the log and out-write the disk. The totals matter as much as the reasons: "0 decoded, 4000 undecodable" and "nothing arriving at all" look identical from the live path, and now they don't. Also replaces the per-frame invalid-timestamp warning with the same counter, and pulls the status strings into constants so the metadata blob and the reporter cannot drift apart.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c43c85bef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type decodeKey struct { | ||
| bus string | ||
| canID int | ||
| status string | ||
| } |
There was a problem hiding this comment.
When multiple p987 vehicles publish through this service, this key and the global success/failure counters merge their outcomes because vehicleID is discarded before reporting. A healthy vehicle's decoded frames can therefore mask that another vehicle has zero successful decodes, while the first warning cannot identify which vehicle failed. Since the subscribed topic and upload-key validation are both vehicle-specific, include the vehicle ID in the report key, totals, and log output.
Useful? React with 👍 / 👎.
1 participant
Debugging "the 987 is publishing CAN frames, they're landing in storage, but nothing appears live" turned out to be near-impossible: a frame that fails to decode produces no signals,
HandleMessagereturns early, and the only record is astatusfield on the stored row. No log, no counter.ok,unknown_can_id,decode_error,invalid_timestamp)(bus, can id, reason)once on first sight, so a new unknown id shows up immediatelyRate matters here — frames arrive at a few hundred per second, so a line per failure would bury the log and out-write the disk. The totals matter as much as the reasons: "0 decoded, 4000 undecodable" and "nothing arriving at all" are indistinguishable from the live path today.
Also folds the per-frame invalid-timestamp warning into the same counter, and pulls the status strings into constants so the metadata blob and the reporter can't drift.
Verified against the real failure modes
Published the CAN id from the car's own stack trace plus an over-length frame:
Both are live concerns on the 987, not hypotheticals.
0x66Cis on the car and absent from the DBC — which by its own header was decoded from a capture of 35 ids but defines only 30. AndFillFromBytesrequires an exact length match where the DBC decoder it replaced accepted anything at or over the declared length, so any message whose real DLC exceeds its DBC length now fails outright.