Skip to content

Fix CYW43439 payload offsets - #1095

Open
sqt wants to merge 1 commit into
ZigEmbeddedGroup:mainfrom
zz-iot:fix/cyw43439-response-offset
Open

sqt wants to merge 1 commit into
ZigEmbeddedGroup:mainfrom
zz-iot:fix/cyw43439-response-offset

Conversation

@sqt

@sqt sqt commented Oct 20, 2026

Copy link
Copy Markdown

Motivation

Payload access depends on the reader’s current position, so reading control data before its header returns incorrect bytes.

Description of changes

  • Calculate payload slices from packet-relative offsets instead of the reader cursor.
  • Add regressions for control payload access before and after header decoding, plus padded WiFi event decoding.

@arkadiuszwojcik

Copy link
Copy Markdown
Contributor

Thanks for the PR!

I noticed that part of this change overlaps with #1093, which was recently merged.

In #1093, I fixed the BDC padding issue by advancing reader.seek past the padding in bdc(). This ensures that, after parsing the BDC header, the reader points to the actual payload. Therefore, the padded Wi-Fi event decoding issue is already fixed by #1093.

However this PR addresses an additional issue: calling data() before cdc() on a control response currently returns incorrect data (we don't do that in code but it is potential scenario), because the reader hasn't advanced past the CDC header yet.

My main concern is about consistency in the parser design.

Currently, Response uses a sequential parsing approach:

  • cdc() consumes the CDC header and advances reader.seek.
  • bdc() consumes the BDC header and its padding, advancing reader.seek to the payload.
  • Both methods cache the parsed headers, so calling them multiple times doesn't advance the reader again.
  • data() returns the remaining bytes based on the current reader position.

This PR changes data() to calculate absolute offsets instead, effectively introducing a second way of locating the payload while the rest of the parser still relies on sequential reads.

Wouldn't it be simpler to keep the existing approach and make data() call self.cdc() in the .control branch, just as it already calls self.bdc() for .event and .data?

Since both header parsers are cached, this would:

  • Fix the call-order dependency between cdc() and data().
  • Preserve the sequential parsing design.
  • Keep the invariant that the reader points to the payload after parsing the relevant header.
  • Avoid duplicating payload-position calculations.

I think calculating absolute offsets is also a valid approach, especially if we eventually want to make Response independent of mutable reader state. But in that case, it might be worth refactoring cdc(), bdc(), and data_pos() together to keep the design consistent.

So I don't consider this PR entirely redundant, but I think the additional issue it addresses could be fixed with a much smaller change while preserving the existing architecture.

So sequence parsing approach fix would look like this:

const head: usize = self.padding() + switch (self.sdp.channel()) {
-       .control => @sizeOf(CDC_Header),
+       .control => blk: {
+           // Ensure the CDC header is parsed so the reader points to the payload.
+           _ = self.cdc();
+           break :blk @sizeOf(CDC_Header);
+       },
        .event, .data => @sizeOf(BDC_Header) + self.bdc().padding(),
    };

What do you think?

This branch has not been deployed

No deployments
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.

2 participants