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

Cyclone and Foxglove possible interop issue for complex appendable types #227

Closed
max-inn opened this issue Sep 24, 2024 · 6 comments
Closed
Labels
bug Something isn't working

Comments

@max-inn
Copy link

max-inn commented Sep 24, 2024

Description
I believe appendable annotation is part of the supported OMGIDL features as I have not come across anything on the readme saying otherwise. If that's not the case then this issue is not relevant.

I noticed a possible interop issue between Foxglove and cyclonedds. We use cyclonedds v0.10.2 and IDL structs serialized with D_CDR2_LE/BE (I believe internally you call it RTPS_DELIMITED_CDR2_LE/BE) fail to deserialize with something like the following:
RangeError: Offset is outside the bounds of the DataView in field a of Inner at location 24. in field inners of Outer at location 24.

I believe this is due to a mismatch in the implementation of the spec as noted in the following issue eclipse-cyclonedds/cyclonedds-cxx#218

  • Version: omgidl-parser/v1.0.3
  • Platform: Ubuntu 22.04

Steps To Reproduce
The following IDL fails to deserialize when the serialized data is generated by cyclone:

@appendable
struct Inner {
  uint32 a;
};
@appendable
struct Outer {
  sequence<Inner> inners;
};

Specifically cyclone will serialize Outer with an empty sequence as:
[0, 9, 0, 0, 8, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0] while Foxglove seems to expect: [0, 9, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0]. I validated this by removing the additional sequence header and Foxglove was able to deserialize the data.

Expected Behavior
I would expect Foxglove to be able to deserialize cyclonedds data.

If the difference in the implementation is the issue, do you think it's possible for Foxglove to align on what cyclone does in this case ? I understand there's different interpretations of the spec so I mainly want to know if this is something I need to address myself rather than wait for a new version.

@max-inn max-inn added the bug Something isn't working label Sep 24, 2024
@jtbandes
Copy link
Member

Thanks for the report, we will look into this issue soon. Any chance you can share some sample code you used to generate the data?

@defunctzombie
Copy link
Contributor

@max-inn
Copy link
Author

max-inn commented Sep 25, 2024

Thanks for the quick response.

Thanks for the report, we will look into this issue soon. Any chance you can share some sample code you used to generate the data?

I've forked the cyclonedds python repo and prepared a minimal example that generates the serialized CDR data I've provided above here.

You have to follow the README instructions to get cyclonedds installed in your python env, then you should be able to run the example from the root of the repo with:
python3 examples/foxglove/foxglove.py

Which should print the serialize data as:
[0, 9, 0, 0, 8, 0, 0, 0, 4, 0, 0, 0, 0, 0, 0, 0]

@snosenzo snosenzo mentioned this issue Oct 18, 2024
1 task
@snosenzo
Copy link
Collaborator

@max-inn Thanks for bringing this up, I have a draft PR that at least fixes the issue with the specific example you mention and doesn't break any of our tests. I still have to test it against some other data in our app, but if that works out hopefully we'll be able to have a fix in the app for you in the next week or so.

snosenzo added a commit that referenced this issue Oct 23, 2024
We weren't writing `DHEADERS` for sequences and arrays that otherwise
lack emheaders.
In the [doc](https://www.omg.org/spec/DDS-XTypes/1.3/PDF) they are
specified as such:
<img width="520" alt="image"
src="https://github.com/user-attachments/assets/5da4fbf7-c759-406f-bb88-bc71346d639e">
<img width="505" alt="image"
src="https://github.com/user-attachments/assets/da2ea6dc-90c9-4466-9e15-3f257340998f">

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:
- [x] test in app against private sample data to make sure no
regressions have occurred.


Linked issues:
FG-8929
#227
@max-inn
Copy link
Author

max-inn commented Oct 28, 2024

That sounds great, thank you !

@snosenzo
Copy link
Collaborator

snosenzo commented Oct 28, 2024

The change has been merged in Foxglove now, it will be in the web deploy by tomorrow and will be in the next desktop release. I'm going to close this issue for now but feel free to reopen or open a new one if you encounter any more issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

4 participants