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

Document and improve wavemap configuration #28

Merged
merged 24 commits into from
Sep 1, 2023

Conversation

victorreijgwart
Copy link
Member

@victorreijgwart victorreijgwart commented Aug 28, 2023

New features:

  • Write documentation page on how to configure wavemap
    • Preview of new docs available here
  • Indicate the unit of config members (e.g. min_cell_width in meters) using types
    • This replaces the previous solution that used enums, which were stored and handled separately
    • With this new solution, the units directly and clearly show up in the documentation
  • Make warnings/errors that can occur when loading configs more descriptive
    • When loading a param fails, print an explanation, including
      • The offending param's name and type
      • The action that will be taken (e.g. it will be ignored, and default value X will be used instead)
  • Update all example configs
  • Define a schema for wavemap configs using jsonschema, which also supports YAML, to enable
    • Text completion, inline documentation, and real-time validation when editing wavemap configs in IDEs like CLion
    • Automatic linting of wavemap configs through .pre-commit hooks
  • Do not emit CMake warnings when the optional livox_ros_driver2 dependency is not found (contributed by astumpf).
    • If users try to initialize a pointcloud input of type livox but the driver is not installed, they will be prompted to install it.
  • Add templates for GitHub issues and pull requests

Notes for the review:

  • The most important changes to review are in the docs/pages directory. The rendered new docs can be explored locally by downloading this archive, unzipping the artifact.tar it contains, and opening the index.html file in a browser.
  • I realize that the usage of exceptions would be more appropriate than using std::optional<Config> return values, but introducing exceptions is a breaking change I would rather properly look into later.
  • If you're pressed for time, the newly added jsonschemas are the least critical to review. These are currently mainly used internally (e.g. to show a warning when an example config is invalid) and don't directly affect users. In general, they just express how the configs are structured and nested. The descriptions they feature for each param are identical to their Doxygen descriptions (also shown on the Configuration page in the docs).

This seems cleaner than applying conditional schemas based on custom properties (e.g. $schema: wavemap), which doesn't work well with CLion and might also break things in other user's pipelines.
@victorreijgwart victorreijgwart self-assigned this Aug 28, 2023
@victorreijgwart victorreijgwart added bug Something isn't working enhancement New feature or request labels Aug 28, 2023
@victorreijgwart victorreijgwart marked this pull request as draft August 28, 2023 11:57
@victorreijgwart victorreijgwart marked this pull request as ready for review August 29, 2023 16:19
@victorreijgwart
Copy link
Member Author

/prepare-release minor

@victorreijgwart victorreijgwart changed the title Update, document and implement linting for config files Document and improve wavemap configuration Aug 30, 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that asking for a solution description is needed or beneficial, as that might just result in unusable ideas. I would however have a section that asks what that feature is supposed to be useful for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rewrote this template from scratch, hopefully it's better now.

Measurement inputs
******************
These settings control the measurement input handlers.
They are nested in the top level config under ``inputs``. Note that ``inputs`` takes a YAML list, and one input handler will be created for each item in the list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is YAML list correct given all this is JSON?

Copy link
Member Author

@victorreijgwart victorreijgwart Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that people will primarily use wavemap with ROS, the settings would be specified in ROS config files (YAML).
The only place where we currently use JSON is when defining the schemas for wavemap configs, which are defined using the jsonschema standard. These schemas are used for linting (pre-commit) and can be used to simplify config editing (in CLion), but don't affect wavemap's runtime.
Based on your comment I briefly considered changing "YAML list" to just "list", but I think it's good to keep the former s.t. users can quickly find the syntax they need for YAML lists on Google. Let me know if you agree/disagree :)

Copy link
Contributor

@LionelOtt LionelOtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation looks good, and I like the entire "make it hard to mess things up with typos and unit errors".

@victorreijgwart
Copy link
Member Author

Thank you @LionelOtt for reviewing, and astumpf for contributing!

@victorreijgwart victorreijgwart merged commit 8c64b98 into main Sep 1, 2023
15 checks passed
@victorreijgwart victorreijgwart deleted the feature/config_schema branch September 5, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants