Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
=======================================
Coverage 36.75% 36.75%
=======================================
Files 138 138
Lines 11207 11207
Branches 5961 5961
=======================================
Hits 4119 4119
Misses 6301 6301
Partials 787 787
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
mojomex
left a comment
There was a problem hiding this comment.
Thank you for the PR!
I am very keen on integrating with Autoware System Designer!
I am however concerned by the sheer number of lines this will introduce (only 4 out of our supported 16 are in this PR, and it's 900 lines already).
Nebula is quite unique since we have a big matrix of sensors with similar-but-not-same parameters, causing huge duplication already in our JSON schemas.
I am not sure what is planned on the tooling front, but treating these files as "lock files" (see Cargo, uv, Yarn, Pixi, etc.) seems the most sensible to me:
We launch each configuration in CI, get available topics, params, etc. and generate the Autoware System Designer files from that.
This way, they are guaranteed to stay up-to-date, and people don't have to manually change
- the Node
- the JSON schema
- the YAML configs
- AND the Autoware System Designer
everytime they make a change to params.
Please let me know your thoughts!
I agree the maintenance cost concern.
I am open to your idea. Please raise those topic to https://github.com/autowarefoundation/autoware_system_designer or https://github.com/autowarefoundation/autoware_system_designer |
|
@mojomex
|
mojomex
left a comment
There was a problem hiding this comment.
I've finally taken the time to take a look 🙇
There are some features/mechanisms that, if added, would make your tool a much better fit with Nebula. I've left some comments about those.
For the param question, we can reasonably expect the user to want to change all of them. I'm not quite sure what the effect of removing param_values has when param_files are also given, but if they can be removed here and still be set by pipelines/architectures, then I'd like to remove all of them as they're already in the config files. The config files are CI-checked agains the parameter schemas, and thus unlikely to fall out of sync with the actual node behavior.
Please let me know your thoughs.
src/nebula_continental/nebula_continental/design/NebulaArs548_decode.node.yaml
Show resolved
Hide resolved
| processes: | ||
| - name: hw_interface | ||
| trigger_conditions: | ||
| - periodic: 100 # Hz |
There was a problem hiding this comment.
While incoming packets are (depending on sensor settings!) 900-3600 Hz, the output pandar_packets are accumulated and published together with pandar_points at 10Hz (or 5, or 20, depending on sensor settings) (so they could be thought of as one process). Please confirm what the best way to express this with your tool is.
There was a problem hiding this comment.
Generally, architecture params that are dependent on node parameters are currently hard to express (e.g. processes.decoder.trigger_conditions.preiodic = param_values.rotation_speed // 60). I think you've talked about the ability to substitute e.g. topic names with parameter values (e.g. for pointcloud concat), so expanding this to a jinja2-style template system that can more accurately describe param dependencies would be nice.
There was a problem hiding this comment.
It is not implemented but I can make the process arguments can be resolved by parameter.
see 5d0e9a9
| - name: blockage_range | ||
| type: string | ||
| default: "[0.0, 0.0]" | ||
| - name: horizontal_ring_id | ||
| type: int | ||
| default: 40 | ||
| - name: is_channel_order_top2down | ||
| type: bool | ||
| default: true | ||
| - name: point_filters.downsample_mask.path | ||
| type: string | ||
| default: "" | ||
| - name: min_azimuth_deg | ||
| type: double | ||
| default: 135.0 | ||
| - name: max_azimuth_deg | ||
| type: double | ||
| default: 225.0 | ||
| - name: polar_voxel_visibility_estimation_mode | ||
| type: string | ||
| default: disable | ||
| - name: vertical_bins | ||
| type: int | ||
| default: 128 | ||
| - name: horizontal_resolution | ||
| type: double | ||
| default: 0.4 | ||
| - name: enable_blockage_diag | ||
| type: bool | ||
| default: true | ||
| - name: blockage_diagnostics_param_file | ||
| type: string | ||
| default: config/blockage_diagnostics_param_file.yaml | ||
| - name: use_dual_return_filter | ||
| type: bool | ||
| default: false |
There was a problem hiding this comment.
A lot of these are not Nebula args but pointcloud preprocessor args. Please double-check with Nebula's parameter schemas. Similarly for the other LiDAR configs.
There was a problem hiding this comment.
parameters that are not used in nebula are removed bdccc21
d039bae to
bdccc21
Compare
mojomex
left a comment
There was a problem hiding this comment.
The cleaned up version already looks much better! The line number became about half of that before 🎉
There are some smaller comments but I think it's close already!
| processes: | ||
| - name: hw_interface | ||
| trigger_conditions: | ||
| - periodic: 600.0 # future: ${parameter rotation_speed} |
There was a problem hiding this comment.
To be precise, it would be something like (2 ** ${param hires_mode}) * (${param return_mode} not in ["First", "Last", "Strongest"]).
Additionally, the actual triggering largely depends on scheduling settings (multiple packets can be processed per trigger).
At this point, I would simplify that to ${param rotation_speed} // 60, which is the output frequency of the packets.
There was a problem hiding this comment.
this process description is mainly for system design and analysis.
it will not effect to operation.
which number do you need when you design sensing system?
I suggest to set a representative number here.
There was a problem hiding this comment.
In that case, it's ${param rotation_speed} // 60 or 10 Hz.
There was a problem hiding this comment.
I turned back to 10 Hz for simplicity.
d9ab3b2
Let's clarify this when further functions utilizing the process definition is developed.
| - name: rotation_speed | ||
| type: float | ||
| default: 600.0 |
There was a problem hiding this comment.
I suppose this was only included for future use in processes below?
There was a problem hiding this comment.
yes. if you don't use, it can be removed.
src/nebula_continental/nebula_continental/design/NebulaArs548_decode.node.yaml
Show resolved
Hide resolved
src/nebula_velodyne/nebula_velodyne/design/NebulaVelodyneVLP16.node.yaml
Outdated
Show resolved
Hide resolved
| use_container: true | ||
| container_name: pointcloud_container |
There was a problem hiding this comment.
These are product-specific and not necessarily how one would use Nebula elsewhere, especially after the introduction of Agnocast.
I think this should be a property of a specific pipeline that uses Nebula, not of Nebula itself.
There was a problem hiding this comment.
I agree.
I set those configuration as PoC.
Future: the container configuration will be set by module.
There was a problem hiding this comment.
launch configuration can be set from module
I implemented this already, but not used. let me try it.
There was a problem hiding this comment.
PR autowarefoundation/autoware_system_designer#30
this PR implements container settings are set from modules and system layer, instead of node.
There was a problem hiding this comment.
use_container and container_name will be removed from node config by autowarefoundation/autoware_system_designer#30
.
…rmat - Introduced a new workflow to lint system design format files on pull requests and pushes to the main branch. - Updated pre-commit configuration to include a new hook for linting with autoware_system_designer. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
- Introduced NebulaArs548_decode.node.yaml and NebulaHesaiPandar128E4X_decode.node.yaml for decoding processes. - Updated NebulaArs548.node.yaml and NebulaHesaiPandar128E4X.node.yaml with new input/output configurations and parameters. - Removed obsolete inputs and outputs in the decode configurations to streamline processing. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…e configurations - Changed launch package for NebulaHesaiPandar128E4X from nebula_ros to nebula_hesai. - Updated parameter file path for NebulaHesaiPandar128E4X. - Introduced new configurations for NebulaVelodyneVLS128, including inputs, outputs, and parameters for the Velodyne sensor. - Added a decoding process for NebulaVelodyneVLS128 with appropriate trigger conditions and outcomes. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
- Introduced NebulaVelodyneVLP16.node.yaml with detailed input/output configurations and parameters for the Velodyne sensor. - Added a new NebulaVelodyneVLP16_decode.node.yaml for decoding processes, specifying inputs, outputs, and parameters. - Streamlined the configuration by removing obsolete outputs and processes in the decoder setup. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
- Changed launch package from nebula_ros to nebula_continental in NebulaArs548.node.yaml to reflect the correct package structure. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…ode configuration - Changed the message type from pandar_msgs/msg/PandarScan to velodyne_msgs/msg/VelodyneScan in NebulaVelodyneVLS128_decode.node.yaml to ensure compatibility with the Velodyne sensor data. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
- Changed the default value of the calibration_file parameter in NebulaVelodyneVLS128.node.yaml to point to the correct calibration file location: $(find-pkg-share nebula_velodyne_decoders)/calibration/VLS128.yaml. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
- Changed 'inheritance' to 'base' in NebulaArs548_decode.node.yaml, NebulaHesaiPandar128E4X_decode.node.yaml, NebulaVelodyneVLP16_decode.node.yaml, and NebulaVelodyneVLS128_decode.node.yaml to align with the updated configuration structure. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…de configuration - Changed the message type from pandar_msgs/msg/PandarScan to velodyne_msgs/msg/VelodyneScan in NebulaVelodyneVLP16_decode.node.yaml to ensure compatibility with the Velodyne sensor data. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…dar128E4X - Bumped autoware_system_design_format from v0.1.0 to v0.2.0 in both NebulaArs548.node.yaml and NebulaHesaiPandar128E4X.node.yaml. - Added package information including name and provider for both nodes to enhance clarity and structure. - Adjusted launch package references to align with the updated package structure. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
- Updated autoware_system_design_format from v0.x.x to 0.x.x in multiple node configuration files for consistency across the project. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…parameter naming - Bumped autoware_system_design_format to 0.3.0 in multiple node configuration files for consistency. - Renamed 'parameters' to 'param_values' and 'parameter_files' to 'param_files' for uniformity across the project. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp> Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp> Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…ebulaHesaiPandar128E4X configuration - Introduced a new parameter 'rotation_speed' with a default value of 600.0. - Updated the trigger condition for 'hw_interface' to use the new 'rotation_speed' parameter. - Modified the 'decoder' process to trigger on 'hw_interface' for improved workflow. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
…28E4X configuration Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
7d61a04 to
7d2930c
Compare
…bulaVelodyneVLS128 configurations - Deleted the 'blockage_mask' publisher entry from both node configuration files to streamline the design. Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
Does it mean NebulaArs584_base is a not-working node? |
- Removed the 'container_name' field from NebulaHesaiPandar128E4X, NebulaVelodyneVLP16, and NebulaVelodyneVLS128 node YAML files for consistency.
- Removed the 'use_container' field from NebulaHesaiPandar128E4X, NebulaVelodyneVLP16, and NebulaVelodyneVLS128 node YAML files for consistency.
Basically, yes. Conceptually it would be the same as an abstract class in object oriented programming. |
…r conditions Signed-off-by: Taekjin LEE <taekjin.lee@tier4.jp>
|
@mojomex can you review the PR please? |
PR Type
Related Links
Please check the issue page autowarefoundation/autoware#6793
Description
describing node information using autoware_system_design_format
Review Procedure
To try autoware_system_designer, please follow instruction here
Remarks
Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks