Support DELIMITED_CDR2
#240
Conversation
…terop-issue-for-complex
@jtbandes oop good catch -- no logic change, but the Cdr writer that wrote that buffer would've been |
| @@ -106,6 +108,7 @@ export class MessageReader<T = unknown> { | |||
| options: HeaderOptions, | |||
| ): Record<string, unknown> { | |||
| const readMemberHeader = options.usesMemberHeader && deserInfo.usesMemberHeader; | |||
| const readDelimiterHeader = options.usesDelimiterHeader && deserInfo.usesDelimiterHeader; | |||
There was a problem hiding this comment.
is there a semantic difference between how you're using uses- and read- prefixes? Or should I think of them as both meaning the same thing, like "delimiter header is [expected to be] present"?
There was a problem hiding this comment.
I guess just the context on which it is used? the uses prefix usually denotes that a field or function that's being called uses that thing. Then inside the function I'm checking those things to figure out if the function should read that thing. So the read is more meant to be read as an imperative for the function inside of itself. I'll admit it's maybe more complicated than it needs to be 😅 I'll see if I can make it a bit clearer
| // new CdrWriter({ kind: EncapsulationKind.RTPS_DELIMITED_CDR2_LE }); | ||
| // would write this | ||
| const buffer = new Uint8Array([0, 9, 0, 0, 8, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0]); |
There was a problem hiding this comment.
so do we not actually support a writer with this encapsulation kind, or why write the buffer manually rather than using the writer class here?
There was a problem hiding this comment.
Honestly the manual buffer is from the example in the issue -- I'm just using it directly for posterity to make sure that it's passing. I can write it with the writer though and check to make sure that it equals this buffer -- I'll also add a comment saying that these test cases are from the cyclone dds issue
| @@ -213,6 +218,10 @@ export class MessageReader<T = unknown> { | |||
|
|
|||
| if (field.typeDeserInfo.type === "struct" || field.typeDeserInfo.type === "union") { | |||
| if (field.isArray === true) { | |||
| // sequences and arrays have dHeaders only when emHeaders were not already written | |||
| if (headerOptions.readDelimiterHeader && !readEmHeader) { | |||
| reader.dHeader(); | |||
There was a problem hiding this comment.
why do we ignore the return value?
There was a problem hiding this comment.
It's not necessary for us since the sequence length is still written. We would use it if we were doing partial deserialization -- in which case we could use it to skip the field entirely
| // if a field is marked as optional it gets an emHeader regardless of emHeaderOptions | ||
| // that would be set by the struct's mutability. |
There was a problem hiding this comment.
nit: this comment now belongs on L174
snosenzo
deleted the
sam/fg-8929-cyclone-and-foxglove-possible-interop-issue-for-complex
branch
### Changelog <!-- Write a one-sentence summary of the user-impacting change (API, UI/UX, performance, etc) that could appear in a changelog. Write "None" if there is no user-facing change --> Add support for delimited appendable sequences and arrays: #240 ### Docs <!-- Link to a Docs PR, tracking ticket in Linear, OR write "None" if no documentation changes are needed. --> ### Description <!-- Describe the problem, what has changed, and motivation behind those changes. Pretend you are advocating for this change and the reader is skeptical. --> <!-- In addition to unit tests, describe any manual testing you did to validate this change. --> <table><tr><th>Before</th><th>After</th></tr><tr><td> <!--before content goes here--> </td><td> <!--after content goes here--> </td></tr></table> <!-- If necessary, link relevant Linear or Github issues. Use `Fixes: foxglove/repo#1234` to auto-close the Github issue or Fixes: FG-### for Linear isses. -->
We weren't writing
DHEADERSfor sequences and arrays that otherwise lack emheaders.In the doc they are specified as such:
These DHEADERs are superseded by EMHEADERs if they're caught beforehand. (I don't have a place in the doc to point at for this but it's been true for historical data we've received.
TODO:
Linked issues:
FG-8929
#227