29-bit CAN, multi-ECU reads, and DTC decoding fixes - #4
Merged
Merged
Conversation
Real-hardware testing over a noisy BLE ELM327 clone kept producing implausible trouble codes (e.g. P0D00) that would come and go across repeated re-reads on the same vehicle with no real fault behind them. parseMultiFrameMessage assembled ISO-TP consecutive frames in receive order with no check that the sequence was complete — a single dropped BLE notification mid-transfer silently shifted every byte after the gap, and extractDataFromFrame's short-data fallback returned the truncated result instead of failing. Both now throw instead of degrading silently: a gap in the 1,2,3.. consecutive-frame sequence, or a final assembly shorter than the length the first frame promised, is a corrupt read, not a partial one to make the best of. Also: `Status` (PID 0101 — MIL + confirmed DTC count + per-monitor readiness) had every field but `dtcCount` non-public despite the struct itself being public, and `StatusTest` wasn't public at all — so a consuming app could physically not read the check-engine-light state or monitor readiness it decoded. Made both fully public.
Broader audit pass after a real-hardware report of "sensors missing again" and a DTC scan failing with an opaque "OBDServiceError error 1" even over WiFi. elm327.swift: getSupportedPIDs' parseResponse took only the FIRST ECU's reply to each supported-PID bitmap request (0100/0120/...) on this vehicle's two-ECU bus. Any PID advertised only by the second ECU was silently absent from OBDInfo.supportedPIDs — and because "first" came from a Dictionary's iteration order, WHICH ecu won wasn't even stable connect to connect, so the missing set could differ each time. Now unions the bitmap across every ECU that answered. wifiManager.swift: processResponse dropped the entire last line whenever it contained the '>' prompt, instead of just the prompt character. A WiFi clone that appends '>' directly onto the last data line with no preceding newline (common on cheap ELM327 emulators) lost that whole line — including real DTC/measurement bytes — while BLE's equivalent path already stripped just the character. Also normalized per-line trimming before the "no data" check, which an untrimmed trailing \r could dodge. Root cause of the opaque error: OBDServiceError, ParserError, CommunicationError, DecodeError conformed to Error but not LocalizedError, and BLEManagerError's CustomStringConvertible.description was never wired to errorDescription — so .localizedDescription on any of them (or anything wrapping them, which is everything OBDService throws) produced only "TypeName error N", the exact opaque message this session hit. Every one of these now surfaces its real message, recursively through underlying errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Label fix: PIDs 49-4B are the accelerator PEDAL sensor per SAE J1979
("Accelerator pedal position D/E/F"), not the throttle plate — the
library described them as "Absolute throttle position D/E/F" (copy-
pasted from 47/48, which really are throttle position). Same 0-100%
linear decode either way, so no vehicle ever saw a wrong number, only
a wrong label. Verified against the Wikipedia OBD-II PID reference
table rather than assumed.
Legacy protocols (J1850 PWM/VPW, ISO9141-2, ISO14230 KWP — used by
pre-CAN vehicles) had the exact same gap-blind assembly bug this
session already fixed for CAN: the generic multi-frame path checked
only that the lowest order byte was 1, not that the whole sequence
was contiguous, so a dropped frame produced a silently-truncated,
shifted response instead of a failure. Now checks every index.
Adapter init now sends ATAT1 (adaptive timing — Elm's own recommendation
for noisy links, growing the per-command timeout from observed bus
response time instead of a fixed one) and ATCAF1 (CAN auto-formatting —
makes explicit the framing assumption every CAN parser in this package
already depends on implicitly). Both best-effort (not `okResponse`):
an older/cheap clone that doesn't recognize either command must not
fail the whole connection over an optional reliability improvement.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Status/StatusDecoder only ever decoded bytes A and B of PID 0101 (MIL, DTC count, and the 3 "continuous" monitors: misfire/fuel system/ components). Bytes C and D — the 8 "non-continuous" monitors — were never even read, despite being present in every compliant 4-byte response. Those are exactly the ones an emissions/smog inspection readiness check actually depends on: catalyst, evaporative system, oxygen sensor, secondary air, EGR/VVT. Verified the exact bit layout against the SAE J1979-derived reference table (Wikipedia's OBD-II PIDs article) rather than guessing: byte C = availability (1 = available), byte D = completion (0 = complete), same polarity as the existing 3, just at bit offset 16-31 instead of 8-15. The 8 fields use spark- ignition (gasoline) semantics as their canonical meaning; the app layer relabels for compression-ignition (diesel) using the same bit positions' differing meaning where that's reliably documented. detectProtocolManually (the fallback sweep used only when the ELM327's own ATSP0 auto-search fails) tried protocols in raw enum-declaration order — 5 legacy protocols before ever reaching CAN. Since MY2008+ US / mid-2000s+ EU vehicles are essentially all CAN, that spent up to 5 full round-trips (ATSPn + 0100 + timeout each) on protocols that were never going to answer first. Now sweeps CAN (6-9) first, then legacy (1-5), then J1939/user CAN (A-C) last. Also: the generic legacy-protocol multi-frame assembler had the same frame-gap blindness the CAN parser did (fixed last commit) — checked only that the lowest order byte was 1, not that the sequence was contiguous. Same fix applied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A background audit of the files this session hadn't yet reviewed found the real dominant cause of "sensors missing again" — bigger than the multi-ECU union fixed earlier today: extractSupportedPIDs always labeled bits as PID 01-20 (`index + 1`), regardless of which of the 6 supported-PID getters (0100/0120/0140/ 0160/0180/01A0) produced the bitmap. Every getter after the first reported its bits under the wrong (already-covered) PID numbers, so anything from PID 0x21 up — fuel level, ambient air temp, control module voltage, fuel type, fuel rate, throttle position B-F, and most of what makes a live-sensor dashboard interesting — could never be recognized as supported, on ANY vehicle, single- or multi-ECU. configureSensors' fallback probe never caught this either, since discovery wasn't empty (PIDs 01-20 alone are enough to populate several groups) — it just silently capped there. Fixed by deriving each getter's block offset from its own command string and adding it before formatting the hex PID label. Also fixed, in priority order: - twosComp(_:length:) masked to `length` bits but never subtracted 2^length for the top half of the range — structurally could never return a negative number. Affects EvapPressureDecoder (PID 0132) and every `signed: true` UAS entry (Mode 6 monitor test values). - getStatus()/requestVin() had the same non-deterministic "only .first ECU" bug the multi-ECU PID fix addressed elsewhere — now prefer the engine ECU like scanDTCs already does. - WiFi sendAndReceiveData treated a TCP EOF mid-response the same as a clean, prompt-terminated one — a dropped connection returned whatever partial bytes had arrived as if they were a complete response. New CommunicationError.connectionClosed distinguishes it. - OBDLogger's minimumLogLevel comparison used OSLogType's raw values directly, which aren't ordered by severity (debug=2 sorts above info=1) — inverted the filter so info/warning were dropped by default while debug passed. Added an explicit severity-rank map. - mockManager's (Simulator-only) multi-frame length calculation added raw hex-character count instead of byte count for the consecutive- frame portion, inflating the declared length ~2x — harmless until this session's stricter parser.swift bounds check started throwing on the now-detectably-wrong length for any multi-PID mock response. - protocol_legacy.swift's order-byte path could index frame.data[2] out of bounds and crash on a truncated frame; now throws instead. All 35 existing package tests still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Real-hardware test against a 2016 Jeep Cherokee (ISO 15765-4, 29-bit ID, protocol 7) came back with zero sensors, no VIN, and every supported-PID query empty — a total regression from the Mustang (11-bit, protocol 6), which works fine. CANParser grouped frames by `Frame.txID`, which masks the address byte with `& 0x07`. That's only meaningful for the 11-bit SAE J1979 functional range (responses 0x7E8-0x7EF, where the low nibble IS the 0-7 ECU index by construction) — it has nothing to do with 29-bit extended addressing, where this vehicle's two ECUs answer from 0x10 and 0x18. Both mask to 0 and collapsed onto the same ECUID bucket, so two independent single-frame replies to one request got merged into a 2-frame group; `Message.init` then tried to decode that as an ISO-TP multi-frame sequence (no `.firstFrame` to anchor on), threw, and silently dropped both ECUs' data — for every request, not just one. Added `Frame.rawAddress` (the untouched address byte) and group by that instead. `txID`/`ECUID` is unchanged and still used for display labels — it just doesn't have to be correct to keep frames from different ECUs apart anymore. All 35 existing tests still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Researched the real addressing conventions rather than assuming the 11-bit fix generalized on its own: - 11-bit (SAE J1979): responses are always 0x7E8-0x7EF — the low nibble directly IS a 0-7 ECU index by construction (ECU assigned 0x7E0-0x7E7 responds at assigned-ID+8). `& 0x07` happens to be exactly right here, and only here. - 29-bit (ISO 15765-4 extended): responses are 0x18DAF1XX where XX is a full, OEM-assigned byte with no fixed range — confirmed this is what broke the Jeep Cherokee (0x10 and 0x18 both mask to 0). - Legacy (ISO 9141-2 / ISO 14230 KWP): per SAE J2178, source address bytes are likewise OEM/tester-assigned, not a small fixed range — same collision risk as 29-bit CAN, just never hit yet on real hardware. `LegacyFrame`/`LegacyParcer` had the identical `txID`-based (`& 0x07`-masked) grouping as the CAN parser did before this session's fix, so applied the same one: added `LegacyFrame.rawAddress` (the untouched source byte) and group by that instead. Also brought the legacy parser's fault-tolerance up to parity with the CAN parser's (which already does this, per its own comment): a single malformed frame or one ECU's frames failing to assemble now drops just that piece instead of throwing and discarding the entire response — `try?` instead of `try` in both `compactMap`s. ECUID's small 4-case enum (engine/transmission/unknown/becm) is intentionally left as a best-effort *label* only, not touched here — OEMs choose their own 29-bit/legacy addresses freely (confirmed via research, not assumed), so there's no universal byte-to-name mapping to encode. Grouping no longer depends on the label being correct; only display does. All 35 tests still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The actual root cause of the 2016 Jeep Cherokee (protocol 7) reading nothing at all — deeper than the ECU-grouping collision fixed in ecf6e46, which was necessary but not sufficient. Every CAN protocol class passed idBits: 11 to the parser, including the 29-bit variants (7, 9) and J1939 (A). Frame.init prepends "00000" padding for 11-bit frames, whose printed header is only 3 hex chars — applying that to an already-full 29-bit line ("18DAF118...", 8 header chars, 24 chars total) produces a 29-char odd-length hex string. hexBytes walks it two chars at a time from index 0, so every byte boundary lands half a nibble off: 12 real bytes become 14 garbage bytes, the 6...12 size guard rejects the frame, and compactMap silently drops it. Result: EVERY frame from a 29-bit vehicle discarded — no VIN, no supported PIDs, no sensors, no DTCs — while protocol detection still "succeeded" because testProtocol greps the raw text for "41 00" without parsing. Protocols 7/9/A now pass idBits: 29. Added a regression test built from the real capture in the Jeep's connection log (two ECUs, 0x10 and 0x18, single-frame replies to 0100) — it locks in both this fix and the rawAddress grouping fix, and documents the single-frame payload convention (PCI + mode echo dropped, trailing pad kept). 36 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The parameter was declared and silently discarded (`retries _:`), making every BLE command exactly one 3-second attempt. Two real consequences, both protocol-wide: - One dropped BLE notification failed the whole read instead of re-asking — a per-read coin flip on a noisy in-car link, and a plausible contributor to the sensor flakiness chased earlier this session. - K-line protocol detection (ISO 9141 / KWP 5-baud init runs 5-10 s inside the ELM327 while it prints "SEARCHING...") could never fit a single 3 s window over BLE, while the WiFi transport honors its retries — the same vehicle would connect over WiFi and fail over BLE for no visible reason. Re-sending after a timeout is safe with the exactly-once completion gate: the timed-out attempt's completion was already consumed, and a late reply to attempt N carries the same payload attempt N+1 awaits. "NO DATA" is deliberately NOT retried — it's the adapter's well-formed "vehicle didn't answer", not a comm failure, and re-asking an unsupported PID three times would burn the live-polling cycle's budget for nothing. 36 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mode 02 returns the snapshot of live values the ECU stored at the moment an emissions DTC set — it stays in memory until codes are cleared, so it applies to codes already stored, not just ones that appear while connected. Which DTC owns the stored frame is already readable via Mode 01 PID 02 (Mode1.freezeDTC, present since forever). Request format is 02 <PID> <frame#>; the response payload matches the Mode 01 layout with one extra frame-number byte after the PID echo, so each PID reuses its own Mode 01 decoder on payload.dropFirst(2). Unsupported/uncaptured PIDs answer NO DATA (single attempt, no retries — that's a well-formed "not stored") and are omitted from the result rather than failing the whole snapshot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Found by reading through icanhack.nl's ISO-TP reference rather than re-deriving from scratch: "if the CAN frame is less than 8 bytes, it can be padded — the spec calls for 0xCC, but 0xAA or 0x55 are common in practice." parseSingleFrameMessage returned everything after the PCI+mode-echo bytes with no regard for the PCI's own declared length, so a padded frame's trailing filler bytes rode along into the decoded data. Harmless for the ordinary Mode 01 measurement decoders — they only ever read fixed byte offsets from the front, so trailing bytes are never touched. Not harmless for DTCDecoder (Mode 03/07/0A), which walks the ENTIRE data length two bytes at a time: a non-zero pad byte pairs up with whatever follows it (another pad byte, or nothing, zero-extended) and decodes as a plausible-looking trouble code that was never actually reported by the vehicle. This is a strong candidate for at least some of the intermittent phantom-DTC reports from earlier this session (P0D00 appearing/disappearing across re-reads) — a separate, independent cause from the frame-sequence-gap bug already fixed, since this one doesn't require a dropped BLE packet at all, just an adapter that pads with anything other than zeros. Regression test built directly from this session's own capture: the 29-bit two-ECU test data (real bytes off a 2016 Jeep Cherokee) turns out to end in exactly this kind of non-zero padding (0xAA, 0x00) — updated its expected payloads to the correctly-truncated 5 bytes (PID-echo + 4-byte supported-PID bitmap, matching the 0100 spec exactly) instead of the 6 bytes the old buggy behavior produced. Added a second, minimal test isolating just the padding-strip behavior against a synthetic Mode 03 single-frame response. 37 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Real-hardware test against the Jeep produced clearly-impossible readings (Engine Run Time = 1,124,073,472 s; Control Module Voltage = 237,559,786 V; Distance w/MIL = 11,184,810 km) alongside a wall of otherwise-normal PIDs timing out repeatedly. Decoding the garbage values to hex was the tell: 1124073472 = 0x43000000 — 0x43 is literally the Mode 3 (GET_DTC) response echo byte — and 11184810 = 0xAAAAAA, the exact non-zero CAN pad byte this session's previous commit just learned to strip from single-frame payloads. Root cause: BLEMessageProcessor's `buffer` was never cleared when a command timed out — only the completion handler slot was reset. A response (or partial response) that arrived just after we gave up waiting for it sat in `buffer` untouched, waiting to be silently prepended onto whichever command's response came next — a stale Mode 3 echo byte or leftover pad byte corrupting a completely unrelated PID's decode. Every command boundary must start from an empty buffer, timeout or not. Fixed by clearing it in the same cancellation handler that resets the completion, and moved every `buffer` touch behind the lock already used for the completion hand-off — the buffer is written from CoreBluetooth's delegate queue and cleared from a Task cancellation handler, two contexts Swift does not guarantee share a queue. Also added missing sanity-check bounds to PIDs this session's own garbage output happened to touch (intake/ambient/oil/manifold temp, catalyst temp x4, MAF, engine run time, distance w/MIL, warm-ups count, control module voltage, direct-inject fuel rail pressure) — they had no declared min/max at all, so the app's own clamp (which treats the exact default 0...100 as "no metadata, don't clamp") never had anything to check against and let any decoded value through regardless of magnitude. This is a safety net on top of the buffer fix, not a replacement for it — every other UAS/temperature PID in the table has the same latent gap and would show the same class of garbage if it hit a similar corruption. 37 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On a two-ECU vehicle (e.g. the 2016 Jeep Cherokee, source addresses 0x10 and 0x18), every single-answer read path — the app's per-PID live-sensor polling (OBDService.sendCommand), freeze frame, getStatus, requestVin — picked its response via `.first` on a Dictionary-ordered message list: a different module from one poll to the next. Live values could alternate between two ECUs' answers, and the earlier `.ecu == .engine` preference (getStatus/requestVin) didn't actually disambiguate on 29-bit buses, where the `& 0x07` label mask maps every module to "engine". New `preferredECUMessage(_:pidEcho:)`: 1. keeps only messages whose first payload byte echoes the requested PID (when given) — discards stale/foreign responses outright; 2. of those, takes the lowest raw source address — the primary engine ECM on BOTH addressing schemes (0x7E8 < 0x7E9... on 11-bit, and 0x10 < 0x18... on 29-bit per SAE J2178). MessageProtocol gains `sourceAddress` (the untouched address byte both Message and LegacyMessage already carried per-frame) to make that possible without leaning on the degenerate ECUID label. 37 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pulled valexa's own continued work on develop since our fork point (a5badae): the getStatus() off-by-one (message.data is [PID,A,B,C,D]; decode() needs .dropFirst() to read A as the MIL byte, not the PID echo) and parseUDS19Data's 3rd DTC byte (keeps distinct ISO 14229 sub-faults of the same base code from collapsing together), plus routing the "NO DATA" case through OBDLogger at debug severity instead of raw error-level logging. One real conflict, in BLEManager.sendCommand: valexa's fix and our own retry-handling addition (this session, e469363) both touched the same catch block for different reasons. Combined rather than picked a side — our retry loop with the noData short-circuit, now also logging that case at debug severity per valexa's fix. elm327.swift's getStatus() auto-merged cleanly with both fixes intact (our preferredECUMessage ECU selection + valexa's dropFirst byte-align). 37 tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
|
Hi @Xandir150, amazing ill have a look, there are some conflicts now, sorry for spotting it so late |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey! I've been building an iOS app around this library and pulled in your
developbranch (PR kkonteh97#43) a while back — it's been solid. Along the way I found and fixed a handful of real bugs while testing against two actual cars (a 2004 Mustang on 11-bit CAN and a 2016 Jeep Cherokee on 29-bit CAN), and figured you'd want them back rather than me just sitting on a fork forever.This branch already has your latest 4 commits merged in cleanly, so it should apply fine on top of where
developis right now.What's fixed:
idBits: 11to the parser, even the 29-bit ones. Run a 29-bit frame through the 11-bit padding logic and you get an odd-length hex string, every byte boundary shifts, and the frame gets rejected. Nothing came back at all from the Jeep until I caught this.& 0x07mask ontxIDonly makes sense for the 11-bit0x7E8-0x7EFrange — on 29-bit, two different ECU addresses (0x10 and 0x18 on the Jeep) both mask to the same value and get merged into one group, which then fails to reassemble. Now grouping by the raw address byte instead.getStatus,requestVin, and live sensor reads all just grabbed.firstfrom a Dictionary, so which ECU "won" varied read to read. Added a small helper that filters by PID echo and picks the lowest source address (the primary ECM either way).getSupportedPIDs()labeled every bitmap as PIDs 01-20 no matter which of the 6 getters produced it, so nothing above 0x20 could ever show up as supported.twosComp()masked the value but never actually subtracted for negative numbers — structurally could never return negative.DTCDecoderwalks the whole length and can turn a padding byte into a made-up trouble code.sendCommandhad aretriesparameter that was just... ignored (retries _:). Now it actually retries.ATAT1/ATCAF1get sent on init now.Tests all pass (37, added a couple with real captured frames for the 29-bit fixes). Happy to split this into smaller PRs if that's easier to review — just say the word.