[WIP] Fix Undercounting of topics in Tools/uorb-graph/create.py

[teyrana]

Problem Description

(updated Sun 14 Mar 2021 10:05:48 AM EDT)

The script at Tools/uorb_graph/create.py was undercounting publications and subscriptions -- particularly C++ definitions.
This PR overhauls & refactors the script to greatly increase the variety of cases it can handle.

Describe your solution

This adds additional fields to the regexes when scanning source code.

• Expands the debugging options of this script
  • adds '-v', '-vv' => increase output verbosity
  • adds '--merge-depends' flag for merging library topics
• Records all warnings -- unhandled or ambiguous matches
  • A simple summary is printed on completion
  • A listing is printed out in verbose/debug output modes
  • only prints out for files found in the supplied source tree (i.e. the --src-path argument)
• Library vs Module
  • By default, modules don't link to their dependent libraries.
  • Dependency libraries may be merged with their modules with the --merge-depends CLI flag.
• Overhauls the regexes used to detect matches
  • Expands several of the regexes to match more topic declarations

Test data / coverage

This patch only affects a developer documentation script, and has no effect on flight code

During development, all test cases were run from the path: /home/teyrana/project/px4/Tools/uorb_graph

Before

Run on master branch @ 9d47f7e

$ ./create.py -o graphviz -s ../../src/
Excluded topics: ['parameter_update', 'mavlink_log', 'log_message']
...
number of modules: 33
number of topics: 5
...

Examples + After Results

1. Module: rc_input

   • Status: Fixed
   • src/drivers/rc_input/RCInput.cpp:264:
     uORB::Publication<vehicle_command_ack_s> vehicle_command_ack_pub{ORB_ID(vehicle_command_ack)};
   • src/drivers/rc_input/RCInput.hpp:146:uORB::Subscription _vehicle_cmd_sub{ORB_ID(vehicle_command)};
   • src/drivers/rc_input/RCInput.hpp:155:uORB::PublicationMulti<input_rc_s> _to_input_rc{ORB_ID(input_rc)};
   • warning count: 0
   • topic count: 8

2. Module: iridiumsbd

   • Status: Fixed
   • src/drivers/telemetry/iridiumsbd/IridiumSBD.h:304:
     uORB::Publication<iridiumsbd_status_s> _iridiumsbd_status_pub{ORB_ID(iridiumsbd_status)};
   • warning count: 0
   • topic count: 1

3. Module frsky_telemetry

   • Status: Fixed
   • example: src/drivers/telemetry/frsky_telemetry/frsky_data.cpp:69:uORB::SubscriptionData<vehicle_acceleration_s> vehicle_acceleration_sub{ORB_ID(veh...
   • warning count: 1
   • topic count: 7

4. Module navigator

   • Status: Fixed
   • src/modules/navigator/geofence.h:187 uORB::SubscriptionData<vehicle_air_data_s> _sub_airdata;
   • src/modules/navigator/mission_block.h:155:uORB::Publication<actuator_controls_s> _actuator_pub{ORB_ID(actuator_controls_2)};
   • src/modules/navigator/navigator_main.cpp:103:_local_pos_sub = orb_subscribe(ORB_ID(vehicle_local_position));
   • src/modules/navigator/gpsfailure.h:79:uORB::Publication<vehicle_attitude_setpoint_s> _fw_virtual_att_sp_pub{ORB_ID(fw_virtual_attitude_setpoint)};
   • warning count: 4
   • topic count: 25

5. Module: temperature_compensation

   • Status: Implemented -- Overcounts lines as warnings which aren't strictly ambiguous, but are harder to process
   • src/modules/temperature_compensation/TemperatureCompensationModule.h:122:uORB::Subscription _baro_subs[BARO_COUNT_MAX]
   • warning sites: 19
   • warning topics: 3
   • topic count: 7

6. Module: commander

   • Status: Fixed
   • src/modules/commander/Commander.hpp:428: uORB::SubscriptionMultiArray<battery_status_s, battery_status_s::MAX_INSTANCES> _battery_status_subs{ORB_ID::battery_status};
   • src/modules/commander/level_calibration.cpp:85: uORB::SubscriptionBlocking<vehicle_attitude_s> att_sub{ORB_ID(vehicle_attitude)};
   • warning count: 15
   • warnings topics: 0
   • topic count: 45

7. Module: vtol_att_control

   • Status: Fixed
   • src/modules/vtol_att_control/vtol_att_control_main.h:135:uORB::SubscriptionCallbackWorkItem _actuator_inputs_fw{this, ORB_ID(actuator_controls_virtual_fw)};
   • src/modules/vtol_att_control/vtol_att_control_main.h:154:uORB::Publication<actuator_controls_s> _actuators_0_pub{ORB_ID(actuator_controls_0)};
   • warning count: 0
   • warnings topics: 0
   • topic count: 21

8. Module (Library): drivers__hott

   • Status: No Change
   • src/drivers/telemetry/hott/messages.cpp:78:_home_sub = orb_subscribe(ORB_ID(home_position));
   • warning count: 6
   • topic count: 6

9. Module hott_telemetry

   • Status: Fixed
   • references 'drivers__hott' library in local parent directory...and that library contains messages
   • without '-c','--combine-dependencies'
     • warning count: 0
     • topic count: 0 (all topics are defined in 'dependency' library)
   • with '-c','--combine-dependencies'
     • warning count: 6
     • topic count: 6

10. Module Nested in another Modules

    • for the nested modules 'simulator' => 'battery_simulator': topics go from 17 => 0
    • how common are nested modules? ... Here's the list:
      • drivers__hott > hott_telemetry # nested AND dependency-linked
      • drivers__hott > hott_sensors # nested but unlinked
      • lightware_laser_serial > lightware_laser_test
      • bmi088 > bmi088_i2c
      • tests > hrt_test
      • controllib > controllib_test
      • cdev > cdev_test
      • mavlink > mavlink_tests
      • uorb_msgs_microcdr ${uorb_sources_microcdr}) > micrortps_client
      • commander > commander_tests
      • simulator > battery_simulator

After: entire source tree

11. Modules: all

    • Scope count: 90
    • Topic count: 106
    • Warning count: 201

12. Modules + Libraries: (w/union):

    • Scope count: 95
    • Topic count: 169

13. Modules + Libraries (w/merge):
    - Scope count: 139
    - Topic count: 171
    - Warning count: 201

[bkueng]

Thanks for looking into this, it's the reason why it was disabled in the docs.
Another part that's currently not handled is pubs/subs in library code.

[teyrana]

FYI: the offending SITL Test (the failing CI check, above) passes when I run it locally. So, I'm not sure what the issues is.

[teyrana]

@bkueng
Test case #5 seems like a dealbreaker, if we're going for 100% coverage.
uORB::Subscription _baro_subs[BARO_COUNT_MAX] {
Extracting the type information from these declarations is probably effort-prohibitive. Basically, at this point, there are two options:

1. parse the AbstractSyntaxTree from a C++ compiler
2. Force the uORB::Subscription to be a template declaration... which forces a codebase-wide refactoring just for the sake of an easier tool.

Obviously, I don't expect either of these to be popular methods.

[bkueng]

Extracting the type information from these declarations is probably effort-prohibitive. Basically, at this point, there are two options:

Yes I agree, I wouldn't go too far either. Using templates has other benefits as well and it's something we might do at some point.
Overall more important to me is that we know what we miss, so e.g. adding a warning for the more complex cases. Or be a bit stricter about allowed syntax for initialization, and raise an error if it's not initialized in class (as this is already the preferred pattern).

There's also a this: https://github.com/PX4/PX4-Autopilot/blob/master/src/modules/commander/Commander.hpp#L428 (ORB_ID::)
And https://github.com/PX4/PX4-Autopilot/blob/master/src/modules/vtol_att_control/vtol_att_control_main.h#L135

[dagar]

Force the uORB::Subscription to be a template declaration... which forces a codebase-wide refactoring just for the sake of an easier tool.

This is actually where I'd like to go (for more than just a tool).

[dagar]

Force the uORB::Subscription to be a template declaration... which forces a codebase-wide refactoring just for the sake of an easier tool.

This is actually where I'd like to go (for more than just a tool).

Please take a look at #16059 if you're interested in this.

[teyrana]

@dagar
Uhhh, I'm actually trying to fix #16620, but I got sidetracked trying to figure out how stuff was organized.

I'll see how far I can push this pr in reasonable time, though.

[teyrana]

@bkueng
Hey some quick vector check questions:

.Q1. Are pseudo-topics "different" from actual topics?:

1. src/modules/navigator/mission_block.h:155
   • uORB::Publication<actuator_controls_s> _actuator_pub{ORB_ID(actuator_controls_2)};
2. src/modules/navigator/gpsfailure.h:79
   • :uORB::Publication<vehicle_attitude_setpoint_s> _fw_virtual_att_sp_pub{ORB_ID(fw_virtual_attitude_setpoint)};

I'm not familiar enough with the uORB machinery -- does it make sense to separate out the RHS topic actuator_controls_2 as a separate topic from the LHS actuator_controls topic? (Either case is easy enough to implement)

.Q2. Are submodules separate

I.e. a module within the directory tree of another module

The code currently treats this as a separate module. Is this the desired behavior.

[teyrana]

Result of running script on entire repo:

graph.fv.pdf

[bkueng]

.Q1. Are pseudo-topics "different" from actual topics?

Yes they are separate instantiations with a common message type (i.e. actuator_controls_s). The graph should use the actual topic (actuator_controls_2). Knowing the type would also be useful, then the links would work too: https://github.com/PX4/PX4-Autopilot/blob/master/Tools/uorb_graph/create.py#L547.

Is this the desired behavior.

Yes, likely. Which one did you have in mind?

[teyrana]

Yes, likely. Which one did you have in mind?

Oh, there are a dozen or so nested modules. I'm just verifying behavior.

A separate issue is how to handle inheriting topics from libraries. (Some of which may be nested) But that is such a big change, I'm not sure it's worth the effort to include -- it involves re-writing a bunch of the parsing code, so It's not clear that it's behaving correctly.

[bkueng]

A separate issue is how to handle inheriting topics from libraries. (Some of which may be nested) But that is such a big change, I'm not sure it's worth the effort to include -- it involves re-writing a bunch of the parsing code, so It's not clear that it's behaving correctly.

Yes as I wrote above. I don't think it's much effort if we go about it like this:

• if inside a module, treat it as part of that module
• if standalone, treat it as separate module (and highlight it differently, for example)

And then we can still assign the libs to the used modules at a later point.

That said, the PR is already an improvement w/o that, so it's fine if you we keep it as is.

[teyrana]

Yes as I wrote above. I don't think it's much effort if we go about it like this:

1. if inside a module, treat it as part of that module
2. if standalone, treat it as separate module (and highlight it differently, for example)
3. And then we can still assign the libs to the used modules at a later point.

That said, the PR is already an improvement w/o that, so it's fine if you we keep it as is.

... Ahhhh, that's not exactly what I was trying to say.... Here's where the PR currently stands:

1. Nested modules are currently treated as separate. This behavior is preserved. For example:
   • src/modules/simulator
   • src/modules/simulator/battery_simulator
2. Standalone modules. Yes. This behavior is preserved.
3. By default, libs are ignored; but on request (--combine cli argument), any libraries included from the px4_add_module function, will added to the including module. (technically, the DEPENDS keyword.) This behavior is new.

This commit implements (3).
5fcacb6

[teyrana]

CI Tests

1. SITL Failures
   • still not sure why these are failing.
   • the failures don't happen at a consistent spot, either.
2. Metadata / uorb_graph
   • this is out-of-date, and needs to be updated.
   • it currently references a startup_file="posix-configs/SITL/init/ekf2/typhoon_h480 which is expected to contain the modules to build a graph of.
   • see old (2017) version from 2017
   • deleted in commit: fe81ccbb9b61c9bae
   • my first thought is to simply delete this call; it is currently 1 of 5 invocations of Tools/uorb-graph/create.py

[bkueng]

By default, libs are ignored; but on request (--combine cli argument), any libraries included from the px4_add_module function, will added to the including module. (technically, the DEPENDS keyword.) This behavior is new.

Ok, that's good. I think we're not very consistent with adding all dependencies.

SITL Failures

These are unrelated.

Metadata / uorb_graph

Right, this needs to be updated. Removing that one is fine.

[teyrana]

@bkueng
Okay, I think this PR is ready for a decision.

Given all the changes are restricted to a single file, I've squashed to a single commit, with a more detailed commit message.
This includes all the bugfixes I can find -- including the "uORB Graph" CI test.

[bkueng]

Nice work

[bkueng]

Is this correct?
What about libraries if we have a scope whitelist?

[teyrana]

Okay, (a) outdated comment -- it should actually be if self._in_scope().

That compound if statement was actually in error -- we shouldn't be filtering on the whitelist at this point -- just if we're in any scope at all.

Will update this.

[bkueng]

Why not keep the existing behavior? Fail if not in a scope.

[teyrana]

Per request, the behavior is changed to warn about line if an ORB_ID was found outside of a containing scope (module or library)

But academic anyway, because I couldn't find any examples of this.

[bkueng]

It's not academic as it will help to raise issues in future refactorings, instead of silently breaking things. It adds robustness and therefore I wanted it to be an assertion.
Especially since this will run as CI and warnings can go unnoticed for quite some time.

[teyrana]

Okay I see your point. I've made this change on #17175 .

[LorenzMeier]

Thanks! Let's get this in as a great first step!

[teyrana]

the rework requests have not made it into this PR.... starting new one.