-
Notifications
You must be signed in to change notification settings - Fork 0
Initial parsable output flags for xcvradm #410
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
base: main
Are you sure you want to change the base?
Conversation
Example output of a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, great work. I have a few small suggestions, but it mostly looks good to me. Thanks!
controller/src/bin/xcvradm.rs
Outdated
MonitorFields::Temperature => monitor | ||
.temperature | ||
.map(|t| t.to_string()) | ||
.unwrap_or_else(|| String::from("unsupported")), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want an empty string here instead of "unsupported". That would make it easier for a program parsing this output to distinguish "there was no temperature / voltage / power to parse" from "I failed to parse this as a decimal value".
Whatever we decide, I think it'd be useful to put in the CLI --help
output what value these take if they're not supported. Module support for these varies widely, so it'll definitely be something we need to make clear how to handle in a parsing program.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I like having an empty string for cases where we do not have a value to report. I updated the help message to include the distinction:
Usage: xcvradm --interface <INTERFACE> monitors [OPTIONS]
Options:
-p, --parseable
Print the output in a parseable format
-o, --output <OUTPUT>
Select the output fields to be displayed. Fields that are not supported by a module are emitted as empty values
Possible values:
- port: The port number of the switch
- temperature: The temperature of the transceiver module
- supply-voltage: Voltage supplied to the transceiver module in V
- average-rx-power: Average received power of the transceiver module in mW
- tx-bias: Transmit bias current of the transceiver module in mA
- tx-power: Transmit power of the transceiver module in mW
- aux1: Auxiliary monitor 1 as reported by the transceiver module
- aux2: Auxiliary monitor 2 as reported by the transceiver module
- aux3: Auxiliary monitor 3 as reported by the transceiver module
--output-separator <OUTPUT_SEPARATOR>
Character used to separate output fields. (Default: ":")
--omit-header
Omit displaying the output header
-h, --help
Print help (see a summary with '-h')
What I am not sure about though is how to handle invalid or unexpected values. We aren't really validating values that this point though, so it is really an "is there a value or not" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I am not sure about though is how to handle invalid or unexpected values. We aren't really validating values that this point though, so it is really an "is there a value or not" case.
I think that's right. If we get to the point of having, say, the supply voltage, then it's "valid" by definition. We don't do any sort of sanity or range checking on that. Assuming we can read the bits from the transceiver in the first place, we only do a scaling and then return the value.
Is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Ok, that makes sense, and tracks with how I understood what we are doing. I think the "empty" or "value" here makes sense then.
This PR adds new flags to the "read" commands of
xcvradm
. The pattern is as followings:-p
--parsable
- Flag to toggle outputting in parsable format-o
--output
- List of comma fields to output. Fields will be output in the order defined, with repeated values allowed.--output-separator
- Separator used to delineate field values.There is still no flag for omitting headers that needs to be added.
This PR should not change the default behavior of any commands.