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

DFReader.py: add bitmask data to verbose print output #937

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

peterbarker
Copy link
Contributor

.. taking the conservative approach to this new feature failing....

@shancock884

@peterbarker
Copy link
Contributor Author

2024-03-28 15:13:19.699: POWR
    TimeUS: 211395439 µs
    Vcc: qnan V
    VServo: qnan V
    Flags: 0
        !BRICK_VALID (main brick power supply valid)
        !SERVO_VALID (main servo power supply valid for FMU)
        !USB_CONNECTED (USB power is connected)
        !PERIPH_OVERCURRENT (peripheral supply is in over-current state)
        !PERIPH_HIPOWER_OVERCURRENT (hi-power peripheral supply is in over-current state)
    AccFlags: 0
        !BRICK_VALID (main brick power supply valid)
        !SERVO_VALID (main servo power supply valid for FMU)
        !USB_CONNECTED (USB power is connected)
        !PERIPH_OVERCURRENT (peripheral supply is in over-current state)
        !PERIPH_HIPOWER_OVERCURRENT (hi-power peripheral supply is in over-current state)
    Safety: 6

2024-03-28 15:13:19.700: RCI2
    TimeUS: 211396039 µs
    C15: 1501 us
    C16: 1501 us
    OMask: 0
    Flags: 1
        HAS_VALID_INPUT (true if the system is receiving good RC values)

@shancock884
Copy link
Contributor

shancock884 commented Apr 15, 2024

Looks nice! A couple of thoughts you are free to consider or ignore :-) ....

I know it is the same as dump_message_verbose in mavutil, but I don't really like the use of "!" for False values.... when I first came across it my eyes read it as a check-mark meaning true (i.e. opposite of what it means)! But maybe its best to remain consistent until someone has a strong enough argument/opinion to change both.

I was thinking it could be good to split the dump_verbose_bitmask function into two: get_field_metadata and dump_verbose_bitmask, as it should make it easier to implement showing enums too in a later PR without looking up metadata twice (and probably needed to do following suggestion...).

It would be nice to show the hex value for bitmasks, alongside the decimal (as dump_message_verbose does). If you do create get_field_metadata, then you can get the field metadata first and do something like if hasattr(field_metadata, 'bitmask'): f.write(" 0x%X (%d)\n" % (val, val)).

@peterbarker
Copy link
Contributor Author

I know it is the same as dump_message_verbose in mavutil, but I don't really like the use of "!" for False values.... when I first came across it my eyes read it as a check-mark meaning true (i.e. opposite of what it means)! But maybe its best to remain consistent until someone has a strong enough argument/opinion to change both.

You wouldn't be surprised to hear I find the ! thing readable, but definitely open to alternatives.

I was thinking it could be good to split the dump_verbose_bitmask function into two: get_field_metadata and dump_verbose_bitmask, as it should make it easier to implement showing enums too in a later PR without looking up metadata twice (and probably needed to do following suggestion...).

It would be nice to show the hex value for bitmasks, alongside the decimal (as dump_message_verbose does). If you do create get_field_metadata, then you can get the field metadata first and do something like if hasattr(field_metadata, 'bitmask'): f.write(" 0x%X (%d)\n" % (val, val)).

Seems reasonable. If you can put those suggestions in patch form I can apply them... otherwise I'll come back to this in a while.

Or I could merge and accept patches later?

@peterbarker
Copy link
Contributor Author

@shancock884 I had another look at your suggestions. I've re-arranged things a little to be somewhat like what you're suggesting, but haven't modified the printing of the value. Too many cases to test for this PR ;-)

Further patches would probably be something like field_metadata = field_metadata_by_name.get(c, None), test for None instead of presence-in-field-metadata-by-name in the call to dump_verbose_bitmask, and then some weird stuff when printing the values. It's complicated by the py2 compat code.

@peterbarker peterbarker merged commit 89e0160 into ArduPilot:master Apr 20, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants