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

uavcan: improve status reporting #23888

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

dakejahl
Copy link
Contributor

@dakejahl dakejahl commented Nov 4, 2024

  • removed mutex from print_status() as this blocks the main loop and causes false positives for HW errors.
  • check if any ESC or Servo output function is set and only print status if at least one is set

@dakejahl dakejahl requested a review from AlexKlimaj November 4, 2024 22:01
@dagar
Copy link
Member

dagar commented Nov 6, 2024

Did you verify absolutely everything is safe without the mutex?

If not entirely sure you could do more granular locking.

@dakejahl
Copy link
Contributor Author

dakejahl commented Nov 7, 2024

Did you verify absolutely everything is safe without the mutex?

If not entirely sure you could do more granular locking.

Yeah I checked everything and it looks like all of these just access member variables. The only risk I see is the array of _functions in the mixer module, but you'd really have to try hard to make it fail and even then I'm not sure it would. Initially I tested with granular locking but it just looks bad. If you want I can add it back, basically just calling unlock/lock after each section.

@github-actions github-actions bot added the stale label Dec 8, 2024
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-jan-22-2025/43384/3

@julianoes julianoes merged commit 1900d2c into PX4:main Jan 21, 2025
49 of 52 checks passed
@dagar
Copy link
Member

dagar commented Jan 22, 2025

What about iterating the nodes here? How could that be safe without taking the mutex?

https://github.com/dakejahl/PX4-Autopilot/blob/eca1c76fbf5e67678bd1bebbd42175607b609fcd/src/drivers/uavcan/uavcan_main.cpp#L1142-L1148

@dakejahl dakejahl deleted the pr-uavcan_status_improve branch January 25, 2025 00:20
@dakejahl
Copy link
Contributor Author

What about iterating the nodes here? How could that be safe without taking the mutex?

https://github.com/dakejahl/PX4-Autopilot/blob/eca1c76fbf5e67678bd1bebbd42175607b609fcd/src/drivers/uavcan/uavcan_main.cpp#L1142-L1148

It's iterating over a fixed length array, only printing node status if the timestamp is non-zero

    template <typename Operator>
    void forEachNode(Operator op) const
    {
        for (uint8_t i = 1; i <= NodeID::Max; i++)
        {
            const NodeID nid(i);
            UAVCAN_ASSERT(nid.isUnicast());
            const Entry& entry = getEntry(nid);
            if (entry.time_since_last_update_ms100 >= 0)
            {
                op(nid, entry.status);
            }
        }
    }
    Entry& getEntry(NodeID node_id) const
    {
        if (node_id.get() < 1 || node_id.get() > NodeID::Max)
        {
            handleFatalError("NodeStatusMonitor NodeID");
        }
        return entries_[node_id.get() - 1];
    }
mutable Entry entries_[NodeID::Max];

JoelJ18 pushed a commit to microstrain-robotics/PX4-Autopilot that referenced this pull request Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants