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

syscmds/param: For -a flag (show all), show also the unused parameters #20529

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

junwoo091400
Copy link
Contributor

Describe problem solved by this pull request

image

PX4 by default was not showing the 'unused' parameters (e.g. Rover control parameters, when running quadcopter modules) that were not provoked / read / set by any module through the param systemcmd interface, unless we were showing the 'non-default' parameter values.

This creates a confusion to the user who isn't aware of this param systemcmd behavior, to think that the parameter doesn't exist (as it can't be found with the param show <NAME> command, but in fact it is only not being shown because we have been specifying to only show the 'used' parameters.

Describe your solution

  • Added pattern wildcard search support for -a flag (which was not possible before)
  • Show a warning when we are only showing used parameters, to inform the user
  • Still support legacy behavior of only showing used parameters when doing a default param show <PARAM_NAME> command, with no flags (makes sense, as long as we have a warning to the user that we are only showing the subset)

Test data / coverage

Before the change
image

After the change
image

Additional context

@potaito this should solve your mystery 😉

- This was causing some confusion, when the PX4 code wansn't using a
specific param during runtime, and a user was unable to view the value
using the `param show` command, since there was no way to enable viewing
unused params, if they were unchanged
- This now enables to show the unused params when the systemcmd is
provoked with the -a flag, as that makes sense
- Also, it adds a note in the param_show function if we print out only
used params, that we are in fact only printing out only 'used' params!
(This will hopefully reduce the confusion among users)
{
if (only_used) {
PARAM_PRINT("Warning! Only parameters used during runtime will be shown\n");
Copy link
Member

Choose a reason for hiding this comment

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

A note should be enough.

Suggested change
PARAM_PRINT("Warning! Only parameters used during runtime will be shown\n");
PARAM_PRINT("Note: only parameters used during runtime will be shown\n");

@dagar
Copy link
Member

dagar commented Nov 2, 2022

This creates a confusion to the user who isn't aware of this param systemcmd behavior, to think that the parameter doesn't exist (as it can't be found with the param show command, but in fact it is only not being shown because we have been specifying to only show the 'used' parameters.

I would be a bit careful here, for all intents and purposes these parameters don't exist (to users).

@junwoo091400
Copy link
Contributor Author

I would be a bit careful here, for all intents and purposes these parameters don't exist (to users).

Right, but we currently don't have any way to manipulate parameters that are not used in run-time, and having a -a flag I think is enough to give user the context (that it will show Parameter that isn't used), since original behavior was printing all the parameters

@junwoo091400 junwoo091400 added the Parameter System Parameter system of PX4 label Mar 24, 2023
@junwoo091400
Copy link
Contributor Author

I would be a bit careful here, for all intents and purposes these parameters don't exist (to users).

What do you think about then preserving the -a case to show all the parameters, but confining the -c flag to show changed parameters that is actually used?

That would enable user to have full access to parameter (-a), meanwhile preventing them from having false notion of parameters that aren't actually used during runtime (-c) as being used 🤔

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.

None yet

3 participants