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

updated docs for holybro as now you need to configure actuators #3626

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

afwilkin
Copy link
Contributor

@afwilkin afwilkin commented Mar 5, 2025

I updated the holybro vehicle file on mainline because of issue: PX4/PX4-Autopilot#24449

Now, whoever is setting up the holybro x500 will have to map actuator output. I wanted the docs to reflect that

Copy link

github-actions bot commented Mar 6, 2025

No flaws found

@hamishwillee
Copy link
Collaborator

Thanks @afwilkin - excellent.

Just to be clear, currently the X500_v2 assigns actuators but does not set the correct default minimum/maximum/idle values, so the motors don't spin in idle. With the change in PX4/PX4-Autopilot#24459 the assignments are removed, so the user has to do the assignment themselves ... and this automatically sets the correct max/min values. Right?

I've added a comment, but doesn't it make more sense to just update the min/max/idle settings. Then the configuration can be used out of the box as was intended, and this change wouldn't be needed. Though we would need to update the line - Assign actuator functions to outputs to match your wiring. since that is the one thing you don't need to do currently (so we would say "unless you do something different".

Shall we wait on those issues to be merged to make sure this is the accepted fix?

@afwilkin
Copy link
Contributor Author

afwilkin commented Mar 6, 2025

@hamishwillee definitely should wait to merge in. Just wanted to make this PR so I didn't leave any loose ends on my part.

We spoke about this in the dev call, but was suggested to delete (if I remember correctly) and allow users to map actuators instead of changing the default min/max values.

Currently the script sets min and max values for pwm (which is 1000-2000), and doing it with the dynamic control allocator sets it to the correct value of 1100-1900.

So I believe it isn't just updating the values, but more about two different ways of setting the disarm/min/max pwms....

Not sure if I explained that correctly... But... See what you are saying. If someone smarter than me tells me I did it wrong I will redo it and update the docs

@hamishwillee
Copy link
Collaborator

hamishwillee commented Mar 6, 2025

So I believe it isn't just updating the values, but more about two different ways of setting the disarm/min/max pwms....

@afwilkin My understanding is slightly different.

The x500 param setup is for a specific vehicle configuration - so the idea is that you can use it more or less plug and play with the X500 build. It should define the correct geometry, actuator mapping, and PWM input/output settings.
What has happened here though is it only maps two out of those three things.

There is nothing to stop you adding the limits to be the correct values - that is exactly what the actuator screen does for you if you assign a function to an output.

[Actually, you wouldn't normally even use the default values - you'd start from the default, then tune them for the supplied ESC, and then those are the values you'd add to the default configuration. Ditto for your other calibration]

The file would then be usable as intended.

Personally I would prefer that approach. But as you say, lets see how it falls out.

Thanks again. Awesome to capture this early as you have done!

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