Expose read/write lengthcode for CDR2 emheaders #20
Conversation
src/lengthCodes.ts
Outdated
| @@ -0,0 +1,36 @@ | |||
| export function getLengthCodeForObjectSize(objectSize: number): number { | |||
There was a problem hiding this comment.
Could be nice if LengthCode was a type? Since it is not actually a freeform number but a specific known set of codes?
src/CdrWriter.ts
Outdated
| @@ -241,37 +252,46 @@ export class CdrWriter { | |||
| // M_FLAG is the value of the Must Understand option for the member | |||
| const mustUnderstandFlag = mustUnderstand ? 1 << 31 : 0; | |||
| // LC is the value of the Length Code for the member. | |||
| let lengthCode: number | undefined; | |||
| switch (objectSize) { | |||
| const finalLengthCode = lengthCode ?? getLengthCodeForObjectSize(objectSize); | |||
There was a problem hiding this comment.
Not clear to me how someone would decide whether to pass in a lengthCode or use this default fallback. I guess it depends on what they are planning to serialize immediately after the header? Maybe this info should be documented in the doc comments
There was a problem hiding this comment.
My thoughts here were that people who don't know how to use lengthCodes can use the default we determine that won't modify serialization, or if they understand lengthCodes they can pass their own, and would hopefully understand the affects on serialization for higher numbers. I will update the comment to be more clear on this.
| if (lengthCode == undefined) { | ||
| // Not currently supporting writing of lengthCodes > 4 | ||
| if (objectSize > 0xffffffff) { | ||
| throw Error(`Object size ${objectSize} for EMHEADER too large. Max size is ${0xfffffffff}`); |
There was a problem hiding this comment.
Did we ever hit this error in the past? Or if not does that mean we are just adding support for this in the writer so that we can use it in our unit tests (we won't actually be using lengthCodes > 4 in production anywhere)?
There was a problem hiding this comment.
We really only use this for unit tests, so we haven't ever hit it before. I don't know about downstream users of this writer though. I'm adding this capability to the writer only for unit tests.
| return defaultLengthCode; | ||
| } | ||
|
|
||
| export const lengthCodeToObjectSizes = { |
There was a problem hiding this comment.
could :just: do 1 << objectSize
There was a problem hiding this comment.
True, but I like having the const type here to reflect the pairing in the spec
| [true, 100, 1, 0], // LC 0, 1 byte | ||
| [false, 200, 2, 1], // LC 1, 2 bytes | ||
| [false, 1028, 4, 2], // LC 2, 4 bytes | ||
| [false, 65, 8, 3], // LC 3, 8 bytes | ||
| [true, 63, 9, 4], // LC 4, any size | ||
| [false, 127, 0xffffffff, 5], // LC 5, any size | ||
| [false, 65, 12, 6], // LC 6, multiple of 4 bytes | ||
| [false, 65, 32, 7], // LC 7, multiple of 8 bytes |
Depends on: foxglove/cdr#20 In PL_CDR2 lengthCode value is used to determine whether the size specified by the emHeader can also be used as the array/string/struct length. This enables that capability. <img width="721" alt="image" src="https://github.com/foxglove/omgidl/assets/10187776/dba861c2-d50c-4db1-a841-b10dfe09c30f">
This was already being read and written, but not exposed for reading or writing. The reason for exposing this now is because for lengthCodes 5-7 it means that it should also be used in place of length where certain members start with an integer length, like for sequenceLength or dHeaders of structs.