Ros2interface exporter enhancement to include timestamp support for VSS signals and custom vspec output#496
Conversation
d60b74e to
9369eef
Compare
|
MoM:
|
9369eef to
4e8602a
Compare
4e8602a to
51ba73c
Compare
* added timestamp to each signal at export time, updated CLI for ros2interface exporter to accept updated params, updated documentation as well. written additional test cases for testing the enhanced functionality. Signed-off-by: Ali Momin <ali_momin@jp.honda> * Updated text_generic.py code to better accomodate the test coverage for ros2interface exporter Signed-off-by: Ali Momin <ali_momin@jp.honda> * added unit tests in test_generic.py file for ros2exporter test coverage boost Signed-off-by: Ali Momin <ali_momin@jp.honda> * removed unnecessary output folder Signed-off-by: Ali Momin <ali_momin@jp.honda> * added line breaks in custom vspec output file between each vspec block Signed-off-by: Ali Momin <ali_momin@jp.honda> * fixed the error of github action where the build was failing due to items[] not being present in the object Signed-off-by: Ali Momin <ali_momin@jp.honda> * removed the description from custom vspec output format Signed-off-by: Ali Momin <ali_momin@jp.honda> * deleted the description key value pair from custom vspec output format Signed-off-by: Ali Momin <ali_momin@jp.honda> * updated the ros2interface exporter to comply with the updated timestamp structure in VSS, updated test cases to comply with the update, updated documentation and generic test test coverage as well Signed-off-by: Ali Momin <ali_momin@jp.honda> * fixed timestamp reference in ros2interface.md Signed-off-by: Ali Momin <ali_momin@jp.honda> * fixed test cases with updated units for both seconds and nanoseconds Signed-off-by: Ali Momin <ali_momin@jp.honda> * update output-vspec and timestamp docs to reflect VehicleDataTypes.Timestamp. Replace inline Timestamp struct description in --output-vspec section with VehicleDataTypes.Timestamp reference. In Document, --timestamp-vspec option is added. Added .msg output examples for both simple and struct timestamp modes. Add dedicated Transformed VSS subsection with canonical output shape. Rewrote Examples section with multi-line formatting and new use cases. Updated Usage synopsis to include --timestamp-vspec. Signed-off-by: Ali Momin <ali_momin@jp.honda> * updated test_generic.py file to comply with updated timestamp syntax Signed-off-by: Ali Momin <ali_momin@jp.honda> * fixed the old references to Time_t in the fallback scenario, updated text_generic.py as well to comply with updated timestamp syntax Signed-off-by: Ali Momin <ali_momin@jp.honda> * refactor(ros2interface): remove simple timestamp mode - Drop --timestamp-mode option, struct-based timestamp is now the only supported format - All generated .msg files emit int64 timestamp_seconds + int64 timestamp_nanoseconds fields - Leaf-mode signal value field is always named `value` (was previously the signal's snake_case tail) - render_get_srv request always uses int64 seconds/nanoseconds pairs - Remove _normalize_timestamp_suffix helper (no longer needed) - Update all tests to reflect new field names and remove --timestamp-mode arguments - Update docs to remove simple-mode references and examples Signed-off-by: Ali Momin <ali_momin@jp.honda> * fixed the output structure text position of custom vspec from core to output section, and some rephrasing Signed-off-by: Ali Momin <ali_momin@jp.honda> * - fixed the code blocks in document and moved out the helping text outside - fixed some punctuation - deleted empty spaces / blanks Signed-off-by: Ali Momin <ali_momin@jp.honda> --------- Signed-off-by: Ali Momin <ali_momin@jp.honda>
* refactored ros2interface.py - resolved issue of timestamp schema from types tree Replace the raw-YAML timestamp resolution approach with a types-tree lookup powered by the already-parsed and validated types_root returned by get_trees(). Changes: - load_vspec_tree now returns (root, types_root) instead of discarding the second value - resolve_timestamp_schema(types_root, struct_name) replaces the four YAML-parsing helpers (_load_yaml_mapping, _extract_timestamp_schema_from_vspec, _candidate_timestamp_vspec_paths, _timestamp_component_comment) - TimestampSchema/TimestampComponent dataclasses trimmed: removed vspec_entry, struct_entry, and source_path fields (unused downstream) - write_transformed_struct_vspec: removed unused timestamp_schema param - CLI: --timestamp-vspec <file> replaced by --timestamp-name <name> (default: "Timestamp"); struct must be declared in --types - Docs updated: new "Timestamp fields" section, updated examples. --------- Signed-off-by: Ali Momin <ali_momin@jp.honda>
b7d4023 to
bda4111
Compare
|
MoM:
|
| fqn = node.get_fqn() | ||
| parts = fqn.split(".") | ||
|
|
||
| for idx in range(1, len(parts)): |
There was a problem hiding this comment.
Maybe connect() helps you? https://github.com/COVESA/vss-tools/blob/master/src/vss_tools/tree.py#L165
There was a problem hiding this comment.
The current loop writes to a plain dict in one pass no object allocation, no tree construction, no mandatory description fields. whereas, connect() builds live VSSNode objects attached to a tree, which would then need to be walked back out to a dict for YAML serialization, it would add extra complexity and allocations for no gain. so, it think a current 3 line loop is good enough.
There was a problem hiding this comment.
which would then need to be walked back out to a dict for YAML serialization
https://github.com/COVESA/vss-tools/blob/master/src/vss_tools/tree.py#L311
it would add extra complexity and allocations for no gain
Not sure here. Acting on plain dicts without using the NodeType enum for instance does not feel right and it feels like reinventing the wheel here
But I don't wanna be the blocking factor that much and feel like I might be too opinionated so all good. No need to adjust I guess
- Used VSSNode.get_node_with_fqn() instead of PreOrderIter loop in resolve_timestamp_schema (removes manual tail-split matching) - Matched timestamp struct by full FQN only. --timestamp-struct-fqn (renamed from --timestamp-name) requires an explicit FQN, no default - Added log.warning when a timestamp property has no ROS type mapping - Rename TimestampSchema → Timestamp, TimestampComponent → TimestampProperty, struct_name → fqn, components → properties - Remove VEHICLE_TIMESTAMP_DATATYPE constant. write_transformed_struct_vspec now uses the fqn directly - Introduced DEFAULT_TIMESTAMP constant with built-in int64 fields. it eliminates None checks in build_timestamp_fields, generate_msgs_*, and render_get_srv - updated the md file accordingly to comply with the changes. Signed-off-by: Ali Momin <ali_momin@jp.honda>
…ns. removed the default fall back option for --timestamp-struct-fqn and re-routed user to error message if incorrect fqn is provided. Signed-off-by: Ali Momin <ali_momin@jp.honda>
| fqn = node.get_fqn() | ||
| parts = fqn.split(".") | ||
|
|
||
| for idx in range(1, len(parts)): |
There was a problem hiding this comment.
which would then need to be walked back out to a dict for YAML serialization
https://github.com/COVESA/vss-tools/blob/master/src/vss_tools/tree.py#L311
it would add extra complexity and allocations for no gain
Not sure here. Acting on plain dicts without using the NodeType enum for instance does not feel right and it feels like reinventing the wheel here
But I don't wanna be the blocking factor that much and feel like I might be too opinionated so all good. No need to adjust I guess
| } | ||
| entries[f"{fqn}.time"] = { | ||
| "type": "property", | ||
| "datatype": VEHICLE_TIMESTAMP_DATATYPE, |
There was a problem hiding this comment.
Shouldn't that be dynamic?
…_generic Signed-off-by: Ali Momin <ali_momin@jp.honda>
Signed-off-by: Ali Momin <ali_momin@jp.honda>
…cases Signed-off-by: Ali Momin <ali_momin@jp.honda>
…estamp_schema Signed-off-by: Ali Momin <ali_momin@jp.honda>
Signed-off-by: Ali Momin <ali_momin@jp.honda>
Signed-off-by: Ali Momin <ali_momin@jp.honda>
Signed-off-by: Ali Momin <ali_momin@jp.honda>
|
@sschleemilch - do you have any more comments on this one? |
sschleemilch
left a comment
There was a problem hiding this comment.
Not 100% but don't want to block further
|
MoM:
|
|
@sschleemilch Thanks for the review and approval. |
The following changes have been made to ros2interface:
https://github.com/COVESA/vehicle_signal_specification/blob/master/spec/include/Timestamp.vspec
https://github.com/COVESA/vehicle_signal_specification/blob/master/spec/VehicleDataTypes.vspec
In line with these changes, the test code has also been updated. For detailed usage of ros2interface, please refer to ros2interface.md.
This PR is related to the following Issue and PR.