Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose read/write lengthcode for CDR2 emheaders #20

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Expose read/write lengthcode for CDR2 emheaders #20

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6cd8a5
return lengthcode from emHeaders for PL_CDR2
snosenzo Nov 14, 2023
474fa1d
remove duplicate test case
snosenzo Nov 15, 2023
c620d48
update comment
snosenzo Nov 15, 2023
11ff35e
address feedback
snosenzo Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/CdrReader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,19 +277,49 @@ Object {
[true, 63, 9],
[false, 127, 0xffffffff],
])(
"round trips EMHEADER values with mustUnderstand: %d, id: %d, and size: %d",
"round trips EMHEADER values with mustUnderstand: %d, id: %d, and size: %d without lengthCode",
(mustUnderstand: boolean, id: number, objectSize: number) => {
const writer = new CdrWriter({ kind: EncapsulationKind.PL_CDR2_LE });

writer.emHeader(mustUnderstand, id, objectSize);

const reader = new CdrReader(writer.data);
// don't want to test default assignment of length code here
const { lengthCode, ...headerNoLengthCode } = reader.emHeader();

expect(headerNoLengthCode).toEqual({
objectSize,
id,
mustUnderstand,
});
// should be defined because of CDR2
expect(lengthCode).toBeDefined();
},
);
it.each([
[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
Comment on lines +300 to +307
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😵‍💫

])(
"round trips EMHEADER values with mustUnderstand: %d, id: %d, size: %d, and lengthCode: %d",
(mustUnderstand: boolean, id: number, objectSize: number, lengthCode: number) => {
const writer = new CdrWriter({ kind: EncapsulationKind.PL_CDR2_LE });

writer.emHeader(mustUnderstand, id, objectSize, lengthCode);

const reader = new CdrReader(writer.data);
const header = reader.emHeader();

expect(header).toEqual({
objectSize,
id,
mustUnderstand,
lengthCode,
});
},
);
Expand Down
26 changes: 17 additions & 9 deletions src/CdrReader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { EncapsulationKind } from "./EncapsulationKind";
import { getEncapsulationKindInfo } from "./getEncapsulationKindInfo";
import { isBigEndian } from "./isBigEndian";
import { lengthCodeToObjectSizes } from "./lengthCodes";
import { EXTENDED_PID, SENTINEL_PID } from "./reservedPIDs";

interface Indexable {
Expand Down Expand Up @@ -185,9 +186,10 @@ export class CdrReader {
}

/**
* Reads the member header (EMHEADER) and returns the member ID, mustUnderstand flag, and object size
* Reads the member header (EMHEADER) and returns the member ID, mustUnderstand flag, and object size with optional length code
* The length code is only present in CDR2 and should prompt objectSize to be used in place of sequence length if applicable.
*/
emHeader(): { mustUnderstand: boolean; id: number; objectSize: number } {
emHeader(): { mustUnderstand: boolean; id: number; objectSize: number; lengthCode?: number } {
if (this.isCDR2) {
return this.memberHeaderV2();
} else {
Expand All @@ -196,7 +198,11 @@ export class CdrReader {
}

/** XCDR1 PL_CDR encapsulation parameter header*/
private memberHeaderV1(): { id: number; objectSize: number; mustUnderstand: boolean } {
private memberHeaderV1(): {
id: number;
objectSize: number;
mustUnderstand: boolean;
} {
// 4-byte header with two 16-bit fields
this.align(4);
const idHeader = this.uint16();
Expand Down Expand Up @@ -260,7 +266,12 @@ export class CdrReader {
}
}

private memberHeaderV2(): { id: number; objectSize: number; mustUnderstand: boolean } {
private memberHeaderV2(): {
id: number;
objectSize: number;
mustUnderstand: boolean;
lengthCode: number;
} {
const header = this.uint32();
// EMHEADER = (M_FLAG<<31) + (LC<<28) + M.id
// M is the member of a structure
Expand All @@ -272,7 +283,7 @@ export class CdrReader {

const objectSize = this.emHeaderObjectSize(lengthCode);

return { mustUnderstand, id, objectSize };
return { mustUnderstand, id, objectSize, lengthCode };
}

/** Uses the length code to derive the member object size in
Expand All @@ -282,13 +293,10 @@ export class CdrReader {
// 7.4.3.4.2 Member Header (EMHEADER), Length Code (LC) and NEXTINT
switch (lengthCode) {
case 0:
return 1;
case 1:
return 2;
case 2:
return 4;
case 3:
return 8;
return lengthCodeToObjectSizes[lengthCode];
// LC > 3 -> NEXTINT exists after header
case 4:
case 5:
Expand Down
78 changes: 49 additions & 29 deletions src/CdrWriter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { EncapsulationKind } from "./EncapsulationKind";
import { getEncapsulationKindInfo } from "./getEncapsulationKindInfo";
import { isBigEndian } from "./isBigEndian";
import { getLengthCodeForObjectSize, lengthCodeToObjectSizes } from "./lengthCodes";
import { EXTENDED_PID, SENTINEL_PID } from "./reservedPIDs";

export type CdrWriterOpts = {
Expand Down Expand Up @@ -184,12 +185,17 @@ export class CdrWriter {
}

/**
* Writes the member header (EMHEADER): mustUnderstand flag, the member ID, and object size
* Writes the member header (EMHEADER): mustUnderstand flag, the member ID, object size, and optional length code for CDR2 emHeaders
* Accomodates for PL_CDR and PL_CDR2 based on the CdrWriter constructor options
*/
emHeader(mustUnderstand: boolean, id: number, objectSize: number): CdrWriter {
emHeader(
mustUnderstand: boolean,
id: number,
objectSize: number,
lengthCode?: number,
): CdrWriter {
return this.isCDR2
? this.memberHeaderV2(mustUnderstand, id, objectSize)
? this.memberHeaderV2(mustUnderstand, id, objectSize, lengthCode)
: this.memberHeaderV1(mustUnderstand, id, objectSize);
}

Expand Down Expand Up @@ -231,7 +237,12 @@ export class CdrWriter {
return this;
}

private memberHeaderV2(mustUnderstand: boolean, id: number, objectSize: number): CdrWriter {
private memberHeaderV2(
mustUnderstand: boolean,
id: number,
objectSize: number,
lengthCode?: number,
): CdrWriter {
if (id > 0x0fffffff) {
// first byte is used for M_FLAG and LC
throw Error(`Member ID ${id} is too large. Max value is ${0x0fffffff}`);
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const header = mustUnderstandFlag | (finalLengthCode << 28) | id;

this.uint32(header);

switch (finalLengthCode) {
case 0:
case 1:
lengthCode = 0;
break;
case 2:
lengthCode = 1;
case 3: {
const shouldBeSize = lengthCodeToObjectSizes[finalLengthCode];
if (objectSize !== shouldBeSize) {
throw new Error(
`Cannot write a length code ${finalLengthCode} header with an object size not equal to ${shouldBeSize}`,
);
}
break;
}
// When the length code is > 3 the header is 8 bytes because of the NEXTINT value storing the object size
case 4:
lengthCode = 2;
case 5:
this.uint32(objectSize);
break;
case 8:
lengthCode = 3;
case 6:
if (objectSize % 4 !== 0) {
throw new Error(
"Cannot write a length code 6 header with an object size that is not a multiple of 4",
);
}
this.uint32(Math.floor(objectSize / 4));
snosenzo marked this conversation as resolved.
Show resolved Hide resolved
break;
case 7:
if (objectSize % 8 !== 0) {
throw new Error(
"Cannot write a length code 7 header with an object size that is not a multiple of 8",
);
}
this.uint32(Math.floor(objectSize / 8));
snosenzo marked this conversation as resolved.
Show resolved Hide resolved
break;
}

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}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

@snosenzo snosenzo Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
lengthCode = 4;
}

const header = mustUnderstandFlag | (lengthCode << 28) | id;

this.uint32(header);

// When the length code is > 3 the header is 8 bytes because of the NEXTINT value storing the object size
if (lengthCode >= 4) {
this.uint32(objectSize);
}

return this;
Expand Down
36 changes: 36 additions & 0 deletions src/lengthCodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
export function getLengthCodeForObjectSize(objectSize: number): number {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be nice if LengthCode was a type? Since it is not actually a freeform number but a specific known set of codes?

let defaultLengthCode;

switch (objectSize) {
case 1:
defaultLengthCode = 0;
break;
case 2:
defaultLengthCode = 1;
break;
case 4:
defaultLengthCode = 2;
break;
case 8:
defaultLengthCode = 3;
break;
}

if (defaultLengthCode == undefined) {
// Not currently supporting writing of lengthCodes > 4
if (objectSize > 0xffffffff) {
throw Error(
`Object size ${objectSize} for EMHEADER too large without specifying length code. Max size is ${0xffffffff}`,
);
}
defaultLengthCode = 4;
}
return defaultLengthCode;
}

export const lengthCodeToObjectSizes = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could :just: do 1 << objectSize

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but I like having the const type here to reflect the pairing in the spec

0: 1,
1: 2,
2: 4,
3: 8,
};
Loading