docs(costmap-filters): add Zone Parameter Filter configuration page#907
docs(costmap-filters): add Zone Parameter Filter configuration page#907aki1770-del wants to merge 7 commits intoros-navigation:masterfrom
Conversation
Companion documentation for ros-navigation/navigation2#6104, which introduces the new ZoneParameterFilter costmap filter plugin. - Add configuration/packages/costmap-plugins/zone_parameter_filter.rst documenting all parameters: enabled, filter_info_topic, transform_tolerance, target_nodes, state_ids, state_<N>.<node>.<param>, nominal_defaults.<node>.<param>, state_event_topic, on_unknown_state, on_param_set_failure. Includes a fully-described YAML example. - Add the new file to the Costmap Filters Parameters toctree in configuration/packages/configuring-costmaps.rst. - Add Zone Parameter Filter row to the Costmap Filters table in plugins/index.rst (table widened to fit the longer plugin name). Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
Five operational sharp edges that surfaced during nav2#6104 design + post-PR audit: - Target-node typos go silent at config-load until first transition - State-to-state transitions only apply the new state's overrides (only state 0 resets to nominal_defaults) - Hot path is non-blocking by design (no wait_for_service) - Effective state-ID range is [1, 127] (int8_t mask transport) - Longest-prefix matching with deterministic length-descending sort Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
| Description | ||
| For each ``N`` in ``state_ids``, declare the parameter overrides to apply when the robot enters mask state ``N``. ``<target_node>`` must be one of the names listed in ``target_nodes``. ``<parameter_path>`` is the parameter name on that target node, dots and all (e.g., ``inflation_layer.inflation_radius``). The parameter type must match the target's declared type. Example: ``state_1.controller_server.FollowPath.max_vel_x: 0.5``. | ||
|
|
||
| :``<filter name>``.nominal_defaults.<target_node>.<parameter_path>: |
There was a problem hiding this comment.
Can we not 'get' each value at startup so we have those rather than configuring them?
…move on_unknown_state Addresses Steve Macenski review on PR ros-navigation#907: - L11 (mechanical phrasing): rewrote the intro paragraph to 4 lines, stripped marketing-style framing. - L18 (int8_t/OCC_GRID_UNKNOWN parenthetical): removed; range stated as [1, 127] plainly. - L24 (AsyncParametersClient configuration paragraph): removed — not a configuration detail. - L70 (target_nodes longest-prefix-match prose): stripped from Description; the matching logic stays in code (see PR #6104 reply). - L116 (on_unknown_state parameter docs entry + YAML example line): removed for coherence with PR #6104 commit e61099a, which removed the parameter from the code (always-throw on unknown mask state per the C.2.a part of the review). Without this, the docs would describe a parameter that no longer exists. - L190 (Common Pitfalls section): renamed to "Notes" and trimmed from 5 bullets to 1 (the state-to-state transition behavior, which is a real configuration consideration). Items deferred to paired commits as the remaining PR #6104 changes ship: - L94 (nominal_defaults framing — depends on auto-capture refactor) - L114 (state_event_topic default — depends on code-side default flip) - L136 (on_param_set_failure — pushback substance restated in reply; docs default-flip pairs with code commit when it lands) Signed-off-by: Akihiko Komada <aki1770@gmail.com>
|
Thank you for reviewing the documents. I pushed changes in commit 7b002d7 that addressed the items not dependent on the in-flight code changes:
Three items were deferred to paired commits as the remaining #6104 changes land:
Please let me know if you have any questions or concerns regarding the changes made. |
…n_param_set_failure defaults Pairs with PR #6104 commits db32b3c (state_event_topic default) and 1cf64e4 (on_param_set_failure default-flip). - L114 (state_event_topic): default flipped from "" to "zone_filter_state"; "Optional" framing dropped; description now states the publisher is always created. - L136 (on_param_set_failure): default flipped from "warn" to "throw"; description rewritten to explain the safety rationale (fail-safe on persistent faults) and the warn-escape for transient-RPC environments. - Example YAML: state_event_topic value updated to match new default; on_param_set_failure value updated to "throw" with comment noting it is the new default. Signed-off-by: Akihiko Komada <aki1770@gmail.com>
…nsistency
Pairs with the PR #6104 Theme B comment scrub and clarifies the answer
to Steve Macenski's L94 inline comment ("Can we not 'get' each value at
startup so we have those rather than configuring them?").
The honest answer is: we tried, and the underlying services::Client
FIFO race makes auto-capture unreliable. The original docs paragraph
already explained this; the production code keeps the declarative-YAML
approach. Trimmed the docs paragraph to match the more concise comment
shape on the cpp side after the PR #6104 Theme B scrub:
- Drops the "Optional but strongly recommended" hedge framing.
- Drops "One entry per parameter..." (self-evident from the parameter
shape declaration above the Description).
- Keeps the race-condition reason in one sentence (the answer to
Steve's question).
- Keeps the warn-on-gap behavior note.
Signed-off-by: Akihiko Komada <aki1770@gmail.com>
| ----- | ||
|
|
||
| - **State-to-state transitions only apply the new state's overrides.** | ||
| If state 1 sets ``A`` and ``B`` and state 2 sets only ``A``, then a 1→2 transition writes the new value of ``A`` and leaves ``B`` at state 1's value. Only the special state ``0`` reset restores anything to ``nominal_defaults``. Plan ``nominal_defaults`` so any param touched by any state has a documented baseline. |
There was a problem hiding this comment.
Maybe we should actually catch this and properly handle it. If Transition A->B, A has things B does not, set back to the default state 0 values.
Basically: at all times all things should be the state 0 except for what is specifically set in the current state's configuration.
|
Other than the 2 open items; LGTM |
Notes block previously admitted the cross-state leak; rewrite to describe the new transition-time reset behavior shipped on nav2 PR #6104. Closes ros-navigation#907 review thread 3211251415. Signed-off-by: Akihiko Komada <aki1770@gmail.com>
…rationale Cite three defenses (FIFO-race, lifecycle service-readiness, async-drain timing) rather than FIFO-only, and link to the Notes-block transition semantics. Closes ros-navigation#907 review threads 3150280806 + 3211253512. Signed-off-by: Akihiko Komada <aki1770@gmail.com>
|
Thanks for the round-2 review. The Notes-block has been rewritten in |
Basic Info
Description of contribution in a few bullet points
configuration/packages/costmap-plugins/zone_parameter_filter.rstdocumenting all 10 parameters of the newZoneParameterFiltercostmap filter plugin (introduced by navigation2#6104). Includes a fully-described YAML example.configuration/packages/configuring-costmaps.rstso the new page renders in the navigation.plugins/index.rstwith the corresponding link to the plugin source. Table column 1 widened from 20 to 26 characters to fit the longer plugin name.Notes for review
Example Fully-Described YAMLblock in the new doc page.