Skip to content

Commit c9e5832

Browse files
committed
fix(protocol): correct forceTrim body, complete gen5 brick set, flag frame-rev-2
Three verified, low-risk protocol-safety fixes: 1. forceTrim (0x19) comment was wrong. It claimed the body is "two LE i32 range args (full erase = both 0xFEFEFEFE)". It isn't: the body is [u32 trim_page][u32 wrap_count] — the same 8 bytes as the HISTORY_END trim token — not a range/erase. Comment corrected; the false 0xFEFEFEFE claim dropped. 2. dangerousCmds vs OpcodeSafety disconnect. dangerousCmds (the ENFORCED never-send set) listed only startUpdateLoad/loadUpdateData/ processUpdateImage = 0x24-0x26, but on gen5 those opcode numbers are NOT IMPLEMENTED — a gen5 strap does not act on them. The gen5 device-update trio is 0x8E/0x8F/0x90 (142/143/144), which lived only in OpcodeSafety.destructive — a classification set that enforces nothing. Added 0x8E/0x8F/0x90 to dangerousCmds so they are actually refused, and tightened comments: 0x24-0x26 are gen4-empirical/unverified numbering (kept, not deletable), 0x8E-0x90 are the gen5 device-update trio, and OpcodeSafety.destructive is classification-only / self-enforces nothing. 3. Frame rev-2 hardening. Frame.packetType/seq/opcode read inner[0]/[1]/[2], assuming rev-1 layout. The gen5 header carries a frame-revision byte at header[1] (0x01 on every real strap frame; see BandProfile.buildHeader); a rev-2 frame stamps 0x02 there, passes both CRCs, yet shifts those field offsets so opcode would silently return a body byte. parseFrame now sets Frame.frameRevOk=false for a gen5 non-rev-1 frame and exposes Frame.decodable (valid && frameRevOk) so callers surface it rather than mis-decode. Non-breaking: gen4 has no revision byte and is always rev-1; frameRevOk defaults true. Tests: +5 (dangerousCmds membership incl. 0x8E/0x8F/0x90; gen5 rev-2 frame is CRC-valid but flagged non-decodable; gen4 always rev-1). dart test 164 pass / 4 skip, dart analyze clean. Also drops two stale doc pointers that reference material not present in this package (commands.dart, gen5_records.dart). Also renames the 0x24-0x26 constants to neutral device-update names (startUpdateLoad/loadUpdateData/processUpdateImage); they are package-internal and nothing outside this repo referenced them.
1 parent 3b93df1 commit c9e5832

7 files changed

Lines changed: 155 additions & 25 deletions

File tree

‎lib/src/commands.dart‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ Uint8List cmdToggleHr(int seq, bool on) =>
159159
///
160160
/// NOTE: sending this with payload `[0x00]` (i.e. `cmdSendR10R11(seq, false)`)
161161
/// is the REAL persistent raw-flood OFF-switch — the off state persists across
162-
/// reconnects. STOP_RAW_DATA (0x52) does nothing. (PROTOCOL_FINDINGS.md:168-169)
162+
/// reconnects. STOP_RAW_DATA (0x52) does nothing.
163163
Uint8List cmdSendR10R11(int seq, bool on) =>
164164
buildCommand(seq, Cmd.sendR10R11Realtime, [on ? 0x01 : 0x00]);
165165
Uint8List cmdToggleImu(int seq, bool on) =>

‎lib/src/constants.dart‎

Lines changed: 40 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class PacketType {
3535
class Cmd {
3636
static const int linkValid = 0x01;
3737
// Report the highest wire-protocol revision the strap understands. Response
38-
// carries the max protocol version — used for firmware/feature gating.
38+
// carries the max protocol version — used for feature gating.
3939
static const int getMaxProtocolVersion = 0x02;
4040
static const int toggleRealtimeHr = 0x03;
4141
static const int reportVersionInfo = 0x07;
@@ -45,8 +45,10 @@ class Cmd {
4545
static const int abortHistoricalTransmits = 0x14;
4646
static const int sendHistoricalData = 0x16;
4747
static const int historicalDataResult = 0x17; // the batch ACK
48-
// DANGER — never send. Body is NOT empty: two LE i32 range args
49-
// (full erase = both 0xFEFEFEFE). See PROTOCOL_FINDINGS.md.
48+
// DANGER — never send. Body is NOT empty and is NOT a range/erase: it is an
49+
// 8-byte `[u32 trim_page LE][u32 wrap_count LE]` — the
50+
// same 8 bytes as the HISTORY_END trim token. It advances the strap's flash
51+
// trim pointer, so a wrong value discards unsynced records.
5052
static const int forceTrim = 0x19;
5153
static const int getBatteryLevel = 0x1A;
5254
static const int rebootStrap = 0x1D; // DANGER
@@ -56,12 +58,18 @@ class Cmd {
5658
static const int setReadPointer = 0x21;
5759
static const int getDataRange = 0x22;
5860
static const int getHelloHarvard = 0x23;
59-
// Firmware-load opcodes (Cmd opcode space — distinct from PacketType 0x24
60-
// COMMAND_RESPONSE, which is inner[0], not a command opcode).
61-
// DANGER — never send (per PROTOCOL_FINDINGS.md destructive list).
62-
static const int startFirmwareLoad = 0x24; // DANGER
63-
static const int loadFirmwareData = 0x25; // DANGER
64-
static const int processFirmwareImage = 0x26; // DANGER
61+
// Device-update trio (Cmd opcode space — distinct from PacketType
62+
// 0x24 COMMAND_RESPONSE, which is inner[0], not a command opcode).
63+
// DANGER — never send.
64+
//
65+
// ⚠ These 0x24-0x26 values are the GEN4-EMPIRICAL numbering and could not
66+
// be confirmed on gen5. A gen5 strap does not act on them; the real gen5
67+
// device-update trio is 0x8E/0x8F/0x90 (= 142/143/144), which
68+
// [dangerousCmds] now also blocks. Keeping the gen4 entries because their
69+
// numbering cannot be disproven, not because it is confirmed.
70+
static const int startUpdateLoad = 0x24; // DANGER (gen4-empirical)
71+
static const int loadUpdateData = 0x25; // DANGER (gen4-empirical)
72+
static const int processUpdateImage = 0x26; // DANGER (gen4-empirical)
6573
static const int sendR10R11Realtime = 0x3F;
6674
// On-device haptic alarm. SET carries a wall-clock epoch + a haptic waveform
6775
// pattern (see cmdSetAlarm in commands.dart for the exact, hardware-verified
@@ -112,7 +120,7 @@ class Cmd {
112120
/// Band-agnostic opcode safety classification, sourced from whoop-rs's
113121
/// hardware-tested command surface (kept SEPARATE from [dangerousCmds] above,
114122
/// which is OpenStrap's own, independently-curated gen4 list — the two do not
115-
/// fully overlap, e.g. this list omits the firmware-load opcodes (0x24-0x26)
123+
/// fully overlap, e.g. this list omits the device-update opcodes (0x24-0x26)
116124
/// that [dangerousCmds] already blocks, and adds a few whoop-rs flags ours
117125
/// didn't have, notably 120/SET_FF_VALUE — see the note on [forbidden] below).
118126
///
@@ -146,24 +154,39 @@ class OpcodeSafety {
146154
};
147155

148156
/// The subset of [forbidden] that is actively destructive (data loss /
149-
/// bricking), not merely "don't auto-fire". Opcodes 142-144 have no named
150-
/// meaning in either reference codebase — treat as permanently blocked,
151-
/// unknown-but-dangerous.
157+
/// bricking), not merely "don't auto-fire". Opcodes 142-144 (0x8E-0x90) are
158+
/// the confirmed gen5 device-update trio — [dangerousCmds]
159+
/// also lists them so they are actually REFUSED, because this set is
160+
/// classification-only and self-enforces NOTHING (see the class doc). Treat
161+
/// membership here as documentation; gate on [dangerousCmds] to block a send.
152162
static const Set<int> destructive = {25, 45, 142, 143, 144};
153163

154164
static bool isForbidden(int opcode) => forbidden.contains(opcode);
155165
static bool isDestructive(int opcode) => destructive.contains(opcode);
156166
}
157167

158-
/// Commands that can brick the link / burn battery / brick flash. NEVER auto-fire.
168+
/// Commands that can brick the link / burn battery / brick flash. NEVER
169+
/// auto-fire. CALLER-ENFORCED: this package builds frames but has no transport
170+
/// write path, so it does not itself block a send — a call site (edge, at the
171+
/// point it writes a command) is expected to check membership here and refuse.
172+
/// This is the set such a guard should gate on (contrast [OpcodeSafety], which
173+
/// only sub-classifies and is likewise not self-enforcing).
159174
const Set<int> dangerousCmds = {
160175
Cmd.forceTrim,
161176
Cmd.togglePersistentR21,
162177
Cmd.rebootStrap,
163178
Cmd.powerCycleStrap,
164-
Cmd.startFirmwareLoad,
165-
Cmd.loadFirmwareData,
166-
Cmd.processFirmwareImage,
179+
// gen4-empirical device-update numbering (unverified; inert on gen5).
180+
Cmd.startUpdateLoad,
181+
Cmd.loadUpdateData,
182+
Cmd.processUpdateImage,
183+
// CONFIRMED gen5 device-update trio (0x8E/0x8F/0x90 =
184+
// 142/143/144). These are the opcodes a real gen5 strap actually acts on, so
185+
// they must be in the enforced set — not merely classified in
186+
// [OpcodeSafety.destructive]. No Cmd.* constant: we never construct these.
187+
0x8E,
188+
0x8F,
189+
0x90,
167190
};
168191

169192
/// Historical-data record type (inner[1] of a 0x2F / data packet).

‎lib/src/control.dart‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -826,6 +826,13 @@ class Decoded {
826826
/// scale, GET_HELLO opcode, historical-record version family); defaults to
827827
/// gen4 so every existing caller is unchanged.
828828
Decoded decodeFrame(Frame frame, {BandProfile profile = BandProfile.gen4}) {
829+
// A frame that passed CRC but advertises a frame revision this decoder does
830+
// not understand (see [Frame.frameRevOk]) must NOT be decoded with the rev-1
831+
// `inner[0]/[1]/[2]` field offsets — surface it instead of silently handing
832+
// back a body byte as the packet type / opcode.
833+
if (!frame.frameRevOk) {
834+
return Decoded('unsupported_frame_rev', {'packet_type': frame.packetType});
835+
}
829836
final inner = frame.inner;
830837
final pt = frame.packetType;
831838
try {

‎lib/src/framing.dart‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,30 @@ class Frame {
2323
final bool crc8Ok;
2424
final bool crc32Ok;
2525

26-
Frame(this.inner, this.crc8Ok, this.crc32Ok);
26+
/// Whether the frame header advertises the frame revision this decoder was
27+
/// written for (rev-1). The `inner[0]/[1]/[2]` = packetType/seq/opcode field
28+
/// offsets below assume rev-1 layout; a rev-2 frame can still pass both CRCs
29+
/// yet shift those fields, so [opcode] would silently return a body byte.
30+
///
31+
/// gen5 carries an explicit revision byte at header[1] (0x01 on every real
32+
/// strap frame — see [BandProfile.buildHeader]); a rev-2 frame stamps 0x02
33+
/// there. gen4's 4-byte header has no revision byte, so it is always treated
34+
/// as rev-1. Defaults true so directly-constructed frames and the entire
35+
/// rev-1 path are unchanged; [parseFrame] sets it false for a gen5 frame
36+
/// whose revision byte is not rev-1 — surfacing it instead of mis-decoding.
37+
final bool frameRevOk;
38+
39+
Frame(this.inner, this.crc8Ok, this.crc32Ok, {this.frameRevOk = true});
2740

2841
/// Band-neutral alias for the header-integrity result.
2942
bool get headerCrcOk => crc8Ok;
3043

3144
bool get valid => crc8Ok && crc32Ok;
45+
46+
/// Safe to read [packetType]/[seq]/[opcode] with the rev-1 field offsets:
47+
/// CRCs pass AND the frame revision is one this decoder understands. Check
48+
/// this (not just [valid]) before trusting [opcode] on an inbound frame.
49+
bool get decodable => valid && frameRevOk;
3250
int get packetType => inner.isNotEmpty ? inner[0] : -1;
3351
int get seq => inner.length > 1 ? inner[1] : -1;
3452
int get opcode => inner.length > 2 ? inner[2] : -1;
@@ -81,7 +99,13 @@ Frame? parseFrame(Uint8List raw, {BandProfile profile = BandProfile.gen4}) {
8199
final storedBd =
82100
raw.buffer.asByteData(raw.offsetInBytes + innerStart + declared - 4, 4);
83101
final stored = storedBd.getUint32(0, Endian.little);
84-
return Frame(Uint8List.fromList(inner), headerCrcOk, stored == crc32(inner));
102+
// gen5 header[1] is the frame-revision byte (rev-1 = 0x01). A rev-2 frame can
103+
// pass both CRCs but shifts the inner field offsets, so flag it rather than
104+
// let the rev-1 getters return a body byte as the opcode. gen4 has no such
105+
// byte and is always rev-1.
106+
final frameRevOk = !profile.isGen5 || raw[1] == revision1;
107+
return Frame(Uint8List.fromList(inner), headerCrcOk, stored == crc32(inner),
108+
frameRevOk: frameRevOk);
85109
}
86110

87111
/// Length-based reassembler. feed() returns every complete Frame it can carve

‎lib/src/gen5_records.dart‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ abstract class Gen5HistoricalRecord {
104104
/// per-field below — several fields have OPEN semantic disagreements between
105105
/// the two reference implementations (whoop-rs vs noop) that could not be
106106
/// resolved from bytes alone; those are called out explicitly rather than
107-
/// silently picking a side. See PROTOCOL_FINDINGS / the multiband spec §1.7.
107+
/// silently picking a side.
108108
class Gen5HistorySample extends Gen5HistoricalRecord {
109109
/// bpm. 0 is a legitimate reading (device warming up), not absence.
110110
final int heartRate;

‎test/framing_test.dart‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,4 +178,80 @@ void main() {
178178
expect(frames.every((f) => f.valid), isTrue);
179179
});
180180
});
181+
182+
group('dangerousCmds (enforced never-send set)', () {
183+
test('includes the confirmed gen5 brick trio 0x8E/0x8F/0x90', () {
184+
expect(dangerousCmds.containsAll(<int>{0x8E, 0x8F, 0x90}), isTrue);
185+
});
186+
187+
test('keeps the gen4-empirical device-update trio + destructive singles', () {
188+
expect(
189+
dangerousCmds,
190+
containsAll(<int>[
191+
Cmd.startUpdateLoad,
192+
Cmd.loadUpdateData,
193+
Cmd.processUpdateImage,
194+
Cmd.forceTrim,
195+
Cmd.rebootStrap,
196+
Cmd.powerCycleStrap,
197+
Cmd.togglePersistentR21,
198+
]),
199+
);
200+
});
201+
202+
test('OpcodeSafety.destructive gen5 opcodes (142-144) are actually enforced', () {
203+
final gen5Destructive =
204+
OpcodeSafety.destructive.where((o) => o >= 142).toSet();
205+
expect(gen5Destructive, {142, 143, 144});
206+
expect(dangerousCmds.containsAll(gen5Destructive), isTrue,
207+
reason: 'classification-only set must be blocked by the enforced set');
208+
});
209+
});
210+
211+
group('frame-rev-2 hardening', () {
212+
test('gen5 rev-2 frame passes CRC but is flagged non-decodable', () {
213+
final raw = Uint8List.fromList(
214+
buildCommand(3, Cmd.getHello, const [0x01], BandProfile.gen5));
215+
216+
final rev1 = parseFrame(raw, profile: BandProfile.gen5)!;
217+
expect(rev1.valid, isTrue);
218+
expect(rev1.frameRevOk, isTrue);
219+
expect(rev1.decodable, isTrue);
220+
221+
// Forge a rev-2 frame: bump the header revision byte (header[1]) and fix
222+
// the header crc16 so the header-integrity check still passes — i.e. the
223+
// frame stays .valid, exactly the latent case that used to mis-decode.
224+
raw[1] = 0x02;
225+
final c = crc16Modbus(raw.sublist(0, 6));
226+
raw[6] = c & 0xFF;
227+
raw[7] = (c >> 8) & 0xFF;
228+
229+
final rev2 = parseFrame(raw, profile: BandProfile.gen5)!;
230+
expect(rev2.valid, isTrue, reason: 'both CRCs still pass');
231+
expect(rev2.frameRevOk, isFalse, reason: 'revision byte 0x02 is not rev-1');
232+
expect(rev2.decodable, isFalse,
233+
reason: 'surfaced, not silently decoded as rev-1');
234+
});
235+
236+
test('gen4 has no revision byte and is always treated as rev-1', () {
237+
final f = parseFrame(buildCommand(0, Cmd.getHelloHarvard, const [0x00]))!;
238+
expect(f.frameRevOk, isTrue);
239+
expect(f.decodable, isTrue);
240+
});
241+
242+
test('decodeFrame surfaces a rev-2 frame instead of decoding rev-1 fields', () {
243+
final raw = Uint8List.fromList(
244+
buildCommand(3, Cmd.getHello, const [0x01], BandProfile.gen5));
245+
raw[1] = 0x02; // bump frame revision
246+
final c = crc16Modbus(raw.sublist(0, 6)); // keep header CRC valid
247+
raw[6] = c & 0xFF;
248+
raw[7] = (c >> 8) & 0xFF;
249+
250+
final f = parseFrame(raw, profile: BandProfile.gen5)!;
251+
expect(f.valid, isTrue);
252+
final d = decodeFrame(f, profile: BandProfile.gen5);
253+
expect(d.kind, 'unsupported_frame_rev',
254+
reason: 'must not be decoded with rev-1 offsets');
255+
});
256+
});
181257
}

‎test/whoop_protocol_update_test.dart‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,14 +102,14 @@ void main() {
102102
Cmd.rebootStrap,
103103
Cmd.powerCycleStrap,
104104
Cmd.togglePersistentR21,
105-
Cmd.startFirmwareLoad,
106-
Cmd.loadFirmwareData,
107-
Cmd.processFirmwareImage,
105+
Cmd.startUpdateLoad,
106+
Cmd.loadUpdateData,
107+
Cmd.processUpdateImage,
108108
]),
109109
);
110110
// 0x24 here is a Cmd opcode; PacketType.commandResponse (0x24) is a
111111
// separate namespace (inner[0], not inner[2]).
112-
expect(Cmd.startFirmwareLoad, 0x24);
112+
expect(Cmd.startUpdateLoad, 0x24);
113113
expect(Cmd.powerCycleStrap, 0x20);
114114
// 0x62 is GET_EXTENDED_BATTERY_INFO; there is no 0x63 command (the old
115115
// "reset fuel gauge" opcode was a decode artifact and has been removed).

0 commit comments

Comments
 (0)