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

control: reduce default control update frequency #37

Merged
merged 2 commits into from
May 16, 2024

Conversation

GuillaumeLaine
Copy link
Member

@GuillaumeLaine GuillaumeLaine commented May 10, 2024

Changes

  • Reduce default control update frequency to 100Hz
  • Do not trigger setSetpointUpdateRateFromSetpointTypes() after registration if the mode has already set a desired setpoint update rate (e.g. already called setSetpointUpdateRate() in the mode constructor)

Testing

  • Tested in SITL that the expected control frequency is set when specifying / not specifying an update rate in the mode's constructor / onActivate

@GuillaumeLaine GuillaumeLaine force-pushed the reduce_default_control_frequency branch from 4681da3 to 61dbceb Compare May 12, 2024 15:51
@GuillaumeLaine GuillaumeLaine requested a review from bkueng May 12, 2024 15:53
@GuillaumeLaine GuillaumeLaine marked this pull request as ready for review May 12, 2024 15:53
px4_ros2_cpp/src/components/mode.cpp Show resolved Hide resolved
@@ -31,7 +31,7 @@ class DirectActuatorsSetpointType : public SetpointBase
~DirectActuatorsSetpointType() override = default;

Configuration getConfiguration() override;
float desiredUpdateRateHz() override {return 500.f;}
float desiredUpdateRateHz() override {return 100.f;}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think these should be lowered. At least not by that much. Direct motor control & rate controller on an MC needs to run fast enough to work correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@markusachtelik was advocating for 100Hz defaults, what is the decision here?

Choose a reason for hiding this comment

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

500 Hz gets the CPU up by 20-30%. in addition, something seems to queue up and px4 says the mode becomes unresponsive.
Rate control is definitely enough at 100 Hz, we can debate 200 Hz for direct motor control. If someone really knows what he/she is doing, the update rate can still be increased. But the default should be something not crazy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The CPU load depends on various factors. I used a rate-controlled mode with this code on Skynode, and didn't have any problems.
I'm fine with lowering it a bit, but 100+200Hz are too low for the general case.

Choose a reason for hiding this comment

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

everything faster than 200Hz (5ms) will not work well on the scheduler on a non RT OS. let's please set this up so it works in the majority of cases by default. If someone wants it faster, there is still the setFrequency() call, which should work now.

can we get this merged please?

(Anecdotal evidence: My team and I have done rate and direct motor controllers with 100 Hz update rate in the past, the latter even flew a flip. Even the quadcopter dynamics are not as crazy as we always think. maybe a very small tiny quad.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just set direct actuators and rates to 200Hz default. Hopefully this is compromising enough, someone feel free to merge once the CI is green (:

@GuillaumeLaine GuillaumeLaine force-pushed the reduce_default_control_frequency branch from ed2b615 to 1f6281b Compare May 13, 2024 08:54
@GuillaumeLaine GuillaumeLaine force-pushed the reduce_default_control_frequency branch from 1f6281b to 138d3a3 Compare May 15, 2024 09:03
@bkueng bkueng merged commit b11608e into main May 16, 2024
9 checks passed
@bkueng bkueng deleted the reduce_default_control_frequency branch May 16, 2024 06:16
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.

3 participants