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.

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

Update value reader and value writer implementation #62

Merged
merged 24 commits into from
Apr 1, 2024

Conversation

sonndinh
Copy link
Member

@sonndinh sonndinh commented Mar 12, 2024

  • Update NodeValueReader and NodeValueWriter to use the lastest changes to ValueReader and ValueWriter interfaces
  • Temporarily disable GitHub Actions jobs that use OpenDDS's latest-release branch (OpenDDS 3.27.0) since it currently doesn't have the changes to ValueReader and ValueWriter (TODO: re-enable those builds when the next OpenDDS release is done)
  • Update NodeDRListener to call ValueDispatcher's write only for samples with valid_data is true (TODO: also pass the control samples to Javascript side when OpenDDS has support for key-only version of vread/vwrite)
  • Correct the DataWriterQos used in test_publisher.js
  • Update the test's security documents to use the updated subject name format in OpenDDS (Update subject name parsing OpenDDS#4201)

@sonndinh sonndinh marked this pull request as ready for review March 26, 2024 20:36
.github/workflows/build.yml Outdated Show resolved Hide resolved

bool NodeValueWriter::write_bitmask(ACE_CDR::ULongLong value, const OpenDDS::DCPS::BitmaskHelper& helper)
{
const OpenDDS::DCPS::String flags = bitmask_to_string(value, helper);
Copy link
Member

Choose a reason for hiding this comment

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

Why is bitmask a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to convert the bitmask to a string formed by the flag names separated by the | character. This string is used for the write instead of a number representation.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like it would be better as an sequence of flag names or even better for javascript, a plain object where each flag has a true or false value. That would make it easier to test if the bitmask had something enabled. With a single string the user would have to process it to find out what it has.

Copy link
Member Author

Choose a reason for hiding this comment

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

This follows the way that JsonValueWriter works. I believe we ended up using this way for JsonValueWriter because its output JSON is mainly used for logging purposes and that's sufficient for manual inspection. Also, within OpenDDS, there is string_to_bitmask function that does the opposite to reconstruct the bitmask.

For NodeValueWriter, I agree having a more structured representation of the bitmask would be easier if the result is going to be processed further. But even with that, there is still type-related information, like the bit positions in the bitmask case, that is not accessible yet on the Javascript side to process the output object.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the bit positions are all that important, but being able to actually get the boolean values would be entire point of the bitmask.

Copy link
Member

Choose a reason for hiding this comment

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

Even in JSON, I'm not sure that format is optimal now that I think about it. If you want to push the JSON through a tool like jq and filter based on a bitmask flag, it would be easier if the flag values were explicit.

@sonndinh sonndinh requested a review from iguessthislldo March 28, 2024 19:25
@jrw972 jrw972 merged commit 0517ddf into OpenDDS:master Apr 1, 2024
12 checks passed
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.

5 participants