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

AP_Motors_test: add tri frames to json output #25670

Merged
merged 2 commits into from
May 21, 2024

Conversation

robertlong13
Copy link
Collaborator

Still need to add more types, but tri frames are a rather common VTOL frame, and this is needed for Mission Planner's motor test page to work.

@robertlong13
Copy link
Collaborator Author

robertlong13 commented Nov 30, 2023

Corresponding Mission Planner PR

While writing this, I noticed that this test order goes against the order for every other frame. Everything else goes clockwise starting from front/front-right). This goes anti-clockwise. Should we change that? I'm guessing there would be push-back to that idea, but I think we should do it.

Nevermind, I'm drafting PRs too late at night. I'm the one that screwed up the order. I didn't match mine with _output_test_seq's order. Fixing now.


for (uint8_t frame_class=0; frame_class <= AP_Motors::MOTOR_FRAME_DECA; frame_class++) {
for (uint8_t frame_type=0; frame_type < AP_Motors::MOTOR_FRAME_TYPE_Y4; frame_type++) {
if (frame_type == AP_Motors::MOTOR_FRAME_TYPE_VTAIL ||
Copy link
Member

Choose a reason for hiding this comment

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

I was skipping these because its a bit un-clear how they should be drawn. The original idea was to generate a diagram. https://discuss.ardupilot.org/t/copter-wiki-motor-diagram-overhaul/101046/24

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that makes sense, but I wanted to unskip them so that they would at least get the right test-order to motor-number relation. The drawing logic has to get manually handled either way, but better to just get this info into the json.

@IamPete1
Copy link
Member

I was planning to just be lazy and add the tricopter ones by hand, but if we can auto generate then even better!

It does not matter for the moment, but if the goal was to make a diagram then the tricopter "plus-revese" frame type should be backwards in pitch.

@robertlong13
Copy link
Collaborator Author

robertlong13 commented Dec 4, 2023

I was planning to just be lazy and add the tricopter ones by hand, but if we can auto generate then even better!

I was originally going to just add them manually, but then noticed this script existed (because the version field in the json) and I assumed that this was the place we wanted these changes done.

It does not matter for the moment, but if the goal was to make a diagram then the tricopter "plus-revese" frame type should be backwards in pitch.

Ah, I missed that. I didn't realize Tris supported that type. I should fix that. In that case, _output_test_seq slightly broken in ArduPilot for these types? Shouldn't we do 4, 7, 1, 2 (edit: noticed that the frame class essentially mirrors the motor diagram, not rotates it, so it should be this instead of 4,7,2,1) for reverse frames to be consistent?

@IamPete1
Copy link
Member

IamPete1 commented Dec 4, 2023

Ah, I missed that. I didn't realize Tris supported that type. I should fix that. In that case, _output_test_seq slightly broken in ArduPilot for these types? Shouldn't we do 4, 7, 2, 1 for reverse frames to be consistent?

Yeah, probably.

libraries/AP_Motors/AP_MotorsTri.cpp Outdated Show resolved Hide resolved
default:
// do nothing
break;
if(!_pitch_reversed) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(!_pitch_reversed) {
if (!_pitch_reversed) {

libraries/AP_Motors/AP_MotorsTri.cpp Show resolved Hide resolved
libraries/AP_Motors/AP_MotorsTri.cpp Outdated Show resolved Hide resolved
libraries/AP_Motors/AP_MotorsTri.h Outdated Show resolved Hide resolved
@robertlong13
Copy link
Collaborator Author

robertlong13 commented Dec 13, 2023

Sorry for all the formatting issues. I've set up astyle now.

@robertlong13
Copy link
Collaborator Author

robertlong13 commented Dec 13, 2023

@IamPete1 I've screwed things up with the #if APM_BUILD_TYPE(APM_BUILD_UNKNOWN), and I'm not sure why. I tried to copy what everything else does, gating only within the cpp file and not the header, but this seems to be leading to "undefined reference to AP_MotorsTri::get_pitch_factor(unsigned char)". Honestly, that's what I expect to see given that I'm not gating the declaration in the header, but somehow we don't get that same issue happening with e.g. AP_MotorsMatrix::get_thrust_rpyt_out. Also, I've tried adding the #if to the header, but it gets very mad about me trying to use APM_BUILD_TYPE there, even with the necessary includes added.

@IamPete1
Copy link
Member

Yeah, this is one of the traps with APM_BUILD_TYPE you can't use it in the header. Because your overriding a existing function your not allowed to define it in the header without having a implementation. The two options are to either decide its OK all the time and not use APM_BUILD_TYPE or to use a different function name so you don't need to override.

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

- fix motor test order for reverse frame
- add frame type string for reverse frame
- fix initialization of _pitch_reversed flag
@IamPete1
Copy link
Member

I have rebased, should get passed the public key error in CI now.

@tridge
Copy link
Contributor

tridge commented May 13, 2024

@robertlong13 will do a bit of RF testing

@tridge tridge merged commit 7e8f9c7 into ArduPilot:master May 21, 2024
91 checks passed
@robertlong13 robertlong13 deleted the pr/ap_motors_json_tri branch May 21, 2024 05:23
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.

6 participants