docs(costmap-filters): add Zone Parameter Filter configuration page#907
Open
aki1770-del wants to merge 5 commits intoros-navigation:masterfrom
Open
docs(costmap-filters): add Zone Parameter Filter configuration page#907aki1770-del wants to merge 5 commits intoros-navigation:masterfrom
aki1770-del wants to merge 5 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>
8 tasks
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>
Comment on lines
+8
to
+11
| mask value at the robot's current pose. The filter is the mechanism; the | ||
| use-case lives in YAML — winter roads, school zones, indoor/outdoor | ||
| transitions, and construction zones all reduce to the same plugin with | ||
| different state maps. |
Member
There was a problem hiding this comment.
The "The filter is the..." to the end of this feels very mechanical. Can you reword to be more clear?
Comment on lines
+15
to
+18
| their YAML-declared nominal values. Each non-zero mask value (effective | ||
| range bounded by the ``int8_t`` ``OccupancyGrid`` transport, ``-128`` to | ||
| ``127``; negative values are reserved for ``OCC_GRID_UNKNOWN`` and are | ||
| ignored) maps to a list of parameter overrides via the layer's |
Member
There was a problem hiding this comment.
Remove in parenthesis content, just list the range plainly and concisely
Comment on lines
+21
to
+24
| Parameter sets are issued asynchronously via ``AsyncParametersClient``; | ||
| the filter never calls ``wait_for_service`` in the hot path. If a target | ||
| node's parameter service is not ready, the filter logs a throttled warning | ||
| and skips that target — the costmap update loop is not blocked. |
Member
There was a problem hiding this comment.
Remove, this is not a configuration detail and pretty clearly written by AI
| ============== ======= | ||
|
|
||
| Description | ||
| Required. Explicit list of target node names the filter is allowed to mutate. The state-override parser does longest-prefix-match against this list, so nested-namespace targets parse unambiguously. Catches typos at config-load. Example: ``["controller_server", "local_costmap"]``. |
Member
There was a problem hiding this comment.
he state-override parser does longest-prefix-match against this list, so nested-namespace targets parse unambiguously.
Huh? Remove perhaps?
| 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>: |
Member
There was a problem hiding this comment.
Can we not 'get' each value at startup so we have those rather than configuring them?
| ====== ======= | ||
|
|
||
| Description | ||
| Optional. If set to a non-empty topic name, the filter publishes a ``std_msgs::msg::UInt8`` message containing the new state ID on every state transition (including transitions to state ``0``). If left empty, no publisher is created. |
Member
There was a problem hiding this comment.
Default to something reasonable and always publish when there are subscriptions. Don't make this "optional"
| ====== ======= | ||
|
|
||
| Description | ||
| Behavior when a target node's ``set_parameters`` call returns ``successful = false``. Either ``"warn"`` (log the failure and continue — the costmap update loop is preserved) or ``"throw"`` (raise an exception, terminating the filter). The default is ``"warn"`` because a hot-path exception in a costmap filter takes the entire navigation stack down; ``"throw"`` is offered for stacks that prefer hard correctness over operational continuity. |
Member
There was a problem hiding this comment.
I question if this should not always throw, since that puts the application in an undefined state.
Comment on lines
+172
to
+190
| Common Pitfalls | ||
| --------------- | ||
|
|
||
| These are the operational sharp edges that have surfaced during design and review: | ||
|
|
||
| - **Target-node typos go silent at config-load until first transition.** | ||
| An override under a ``state_<N>`` namespace whose first segment doesn't match an entry in ``target_nodes`` is rejected with a warning. The filter still becomes active and continues with the remaining valid overrides — but the typo'd parameter is never set. Always check the warn log on first activation. | ||
|
|
||
| - **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. | ||
|
|
||
| - **The hot path is non-blocking by design.** | ||
| If a target node's parameter service is not ready when ``process()`` runs (target node restarting, network blip, etc.), the override for that node is skipped that cycle and a throttled warning is logged. The filter does *not* call ``wait_for_service`` because that would stall the entire costmap update loop. Subsequent ``process()`` calls retry naturally. | ||
|
|
||
| - **Effective state-ID range is [1, 127], not [1, 255].** | ||
| Although the documentation shows the conceptual range as 0–255, the underlying ``OccupancyGrid::data`` field is ``int8_t``, so values 128–255 arrive as their signed-negative complement. Negative mask values are reserved for ``OCC_GRID_UNKNOWN`` and are silently ignored at the robot's pose. If you need the full 0–255 range, wrap your mask publisher to reinterpret-cast unsigned to signed before publishing — but [1, 127] covers the vast majority of zone schemes. | ||
|
|
||
| - **Longest-prefix matching applies to ``target_nodes`` ordering.** | ||
| When ``target_nodes`` contains both a node name and a namespace-prefixed sibling (e.g., ``["a", "a.b"]``), an override under ``state_<N>.a.b.foo`` is routed to ``a.b`` (longer match), not to ``a`` (with parameter path ``b.foo``). The filter sorts ``target_nodes`` by length descending at config-load to make this deterministic. |
Member
There was a problem hiding this comment.
Are any of these real things that we should be pointing out? I don't see a ton of useful context here other than obvious "typos are bad" remarks.
…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>
Author
|
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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.