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

C++: add missing implementations for client advertised schema/encoding #876

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

paulsohn
Copy link
Contributor

@paulsohn paulsohn commented Nov 13, 2024

Changelog

C++ server/client: add missing implementations for client advertised schema / schema encoding

Docs

Description

Although foxglove ws protocol allow client to advertise complete schema definition string as well as schema name, current C++ implementation lacks such fields. Particularly:

  • ClientAdvertisement struct lacks schemaEncoding field,
  • ClientAdvertisement struct has schema field, but it is never populated (websocket server does not read schema or schema encoding to json)
  • websocket client is does not serialize schema or schema encoding to json

This PR resolves above problems, and additionally since this is breaking change from unimplemented to implemented, types are decided as following:

  • schema field type is changed from std::vector<uint8_t> to std::optional<std::string>, as schema fields are passed in as json quoted string, and it's optional
  • schemaEncoding field type is determined as std::optional<std::string>, the same reason for schema.

Fixes: #875
Fixes: FG-9447

@paulsohn paulsohn marked this pull request as draft November 13, 2024 07:51
@defunctzombie
Copy link
Contributor

@paulsohn Thanks for the proposed fix here - let us know when this is ready for review!

@paulsohn
Copy link
Contributor Author

@defunctzombie Sure.
It is a draft because I got no tests but I don't think further changes might be needed, so feel free to test this as it is (I will do my tests in next week)

@paulsohn
Copy link
Contributor Author

paulsohn commented Nov 19, 2024

@defunctzombie Fixed error so now this is properly ready for review.

Added some example in separate branch ( https://github.com/paulsohn/ws-protocol/tree/test-pr876 ).
After build, execute make example_server_client_pub and make example_client_pub_json (or ..._protobuf) on separate shell in that order to test.

@paulsohn
Copy link
Contributor Author

Are you currently reviewing this? If you don't mind, let me know the progress so far

By the way I just thought, it might affect ros-foxglove-bridge as well?

https://github.com/foxglove/ros-foxglove-bridge/blob/main/ros2_foxglove_bridge/src/ros2_foxglove_bridge.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

C++ type definition doesn't contain schemaEncoding field
2 participants