Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Flatbuffer Schema Support #70
Add Flatbuffer Schema Support #70
Changes from 4 commits
15c29ff
5f2e676
058467e
d345d27
d3ac125
ff4b563
e274c21
5e38b8b
c251616
5e11a5c
45fc71a
3909e08
6104a53
3a6a8a5
fa625ec
141de2e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use the
int64
andint32
aliases for clarity?Maybe worth copying/adapting this from protobuf docs to give more explanation on how negative durations work?
On the other hand, an option would be to simply use a single uint64 (time) or int64 (duration). That would give a wider range of representable values too, although it probably doesn't matter because the range is so large already.
Note that ROS does it differently:
ROS 2:
int32 sec, uint32 nsec: https://github.com/ros2/rcl_interfaces/blob/master/builtin_interfaces/msg/Time.msg
int32 sec, uint32 nsec: https://github.com/ros2/rcl_interfaces/blob/master/builtin_interfaces/msg/Duration.msg
C++ API:
Time (int32_t seconds, uint32_t nanoseconds
https://docs.ros2.org/beta3/api/rclcpp/classrclcpp_1_1Time.htmlC++ API:
Duration (int32_t seconds, uint32_t nanoseconds)
https://docs.ros2.org/dashing/api/rclcpp/classrclcpp_1_1Duration.htmlC API: uint64 time, int64 duration, both storing nanoseconds: https://docs.ros2.org/beta1/api/rcl/time_8h.html#a5957493a6bde7c3878f947398f048a24
ROS 1:
uint32 sec, uint32 nsec http://docs.ros.org/en/latest/api/rostime/html/classros_1_1TimeBase.html
int32 sec, int32 nsec http://docs.ros.org/en/latest/api/rostime/html/classros_1_1DurationBase.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good call, added the comment in line with the protobuf.
What kind of consideration should be made to the ROS times schemas in this context? Is this difference going to cause issues at read time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here because it's not really specific to flatbuffers. If we want to add defaults to some fields, we need to add the default values to
schemas.ts
and just read them here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to the schema under
defaultValue?: string
under theMessageSchemaField
type if that seems appropriateThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use ubyte for enums? Might want to add validation that the values all fit within the range of the type we choose. I assume the compiler would validate this, so if we can get the compiler to be tested in CI, then that would be sufficient.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.