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

Fix : Log discrepancy when setting MPPI parameters dynamically #4707

Closed
wants to merge 2 commits into from

Conversation

PlatHum
Copy link

@PlatHum PlatHum commented Oct 4, 2024


Basic Info

Info Please fill out this column
Ticket(s) this addresses #4704
Primary OS tested on Ubuntu
Robotic platform tested on -
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • If ParametersHandler::dynamicParamsCallback cannot find the dynamically set parameter name in its parameters list, set the ParametersResult output message's successful field to false instead of always true.

Future work that may be required in bullet points

  • The rcl_interfaces/msg/SetParametersResult message returned also has a reason field that could be populated with the reason for failure. This could be the name of the parameter not found or the full already printed warning.

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@PlatHum PlatHum changed the title Fix : Log discrepancy when setting MPPI parameters dynamically #4704 Fix : Log discrepancy when setting MPPI parameters dynamically Oct 4, 2024
@SteveMacenski
Copy link
Member

SteveMacenski commented Oct 5, 2024

@PlatHum can you rebase on or pull in main? Its failing with a bunch of tests and I'm not sure why. We did update the cache recently so I'm curious if that's why. If that doesn't work and this is still failing several tests, there may be downstream implications of this PR that we didn't consider. I just retriggered the job for a second time, lets see what it does.

Signed-off-by: Francisco Gonçalves <[email protected]>
@PlatHum
Copy link
Author

PlatHum commented Oct 5, 2024

@PlatHum can you rebase on or pull in main? Its failing with a bunch of tests and I'm not sure why. We did update the cache recently so I'm curious if that's why. If that doesn't work and this is still failing several tests, there may be downstream implications of this PR that we didn't consider. I just retriggered the job for a second time, lets see what it does.

I think this reveals a previously unkown/uncaught error with dynamic parameter setting, perhaps? Or at least a problem with the tests performed. Previously, setting parameters always returned success even if the parameter was not set successfully or not found, which made it seem that all was fine.

From the logs of one of the tests as an example:

    [controller_server-8] [WARN] [1728127317.967062716] [controller_server]: Parameter controller_server.verbose not found
    [controller_server-8] [INFO] [1728127317.969753939] [controller_server]: Optimizer reset
    [controller_server-8] [ERROR] [1728127317.969881341] [controller_server]: Caught exception in callback for transition 13
    [controller_server-8] [ERROR] [1728127317.969913634] [controller_server]: Original error: parameter 'controller_server.verbose' could not be set: 
    [controller_server-8] [WARN] [1728127317.969939448] [controller_server]: Error occurred while doing error handling.
    [controller_server-8] [FATAL] [1728127317.969949455] [controller_server]: Lifecycle node controller_server does not have error state implemented
    [lifecycle_manager-17] [ERROR] [1728127317.970300630] [lifecycle_manager_navigation]: Failed to change state for node: controller_server
    [lifecycle_manager-17] [ERROR] [1728127317.970356458] [lifecycle_manager_navigation]: Failed to bring up all requested nodes. Aborting bringup.

For this one in particular, it seems like ParametersHandler::dynamicParamsCallback does not recognize verbose as one of its parameters, which it should.

@SteveMacenski
Copy link
Member

SteveMacenski commented Oct 7, 2024

Please look into this - this is causing not only the individual unit tests to fail, but also the full system tests that use MPPI, so this PR would not allow MPPI to work. Have you tested MPPI after making these changes?

Its worth checking where this is being triggered, it may well be valid sometime in setup / configuration - considering we see error logs around lifecycle transition failures. I believe when nodes are declared, they are set, and thus the callback could be triggered.

@SteveMacenski
Copy link
Member

@PlatHum would you agree that #4711 supersedes?

@PlatHum
Copy link
Author

PlatHum commented Oct 7, 2024

@PlatHum would you agree that #4711 supersedes?

Yes. @aosmw figured out that the "verbose" parameter needed to be declared before the dynamic parameter handler. If it passed the tests, that fixes it.
Can/Should I close this PR or is it you that takes care of closing?

@SteveMacenski
Copy link
Member

I can close - but we appreciate your proactivity and help! I hope to merge another one of your PRs soon 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log discrepancy when setting MPPI parameters dynamically
2 participants