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.

Sign up for GitHub

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

Jump to bottom

AP_Motors_test: add tri frames to json output #25670

Merged
merged 2 commits into from
May 21, 2024
Merged

AP_Motors_test: add tri frames to json output #25670

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 robertlong13 mentioned this pull request Nov 30, 2023
Open
@robertlong13
Copy link
Collaborator Author

robertlong13 commented Nov 30, 2023 •
edited
Loading

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.

IamPete1
IamPete1 reviewed Nov 30, 2023
View reviewed changes
libraries/AP_Motors/examples/AP_Motors_test/AP_Motors_test.cpp

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 •
edited
Loading

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.

@robertlong13 robertlong13 force-pushed the pr/ap_motors_json_tri branch 3 times, most recently from db4d1f9 to e927627 Compare December 11, 2023 01:09
IamPete1
IamPete1 reviewed Dec 12, 2023
View reviewed changes
libraries/AP_Motors/AP_MotorsTri.cpp Outdated Show resolved Hide resolved
libraries/AP_Motors/AP_MotorsTri.cpp Outdated
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
libraries/AP_Motors/examples/AP_Motors_test/AP_Motors_test.cpp Outdated Show resolved Hide resolved
@robertlong13
Copy link
Collaborator Author

robertlong13 commented Dec 13, 2023 •
edited
Loading

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

@robertlong13
Copy link
Collaborator Author

robertlong13 commented Dec 13, 2023 •
edited
Loading

@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.

@robertlong13 robertlong13 force-pushed the pr/ap_motors_json_tri branch from e7573a9 to a143b31 Compare May 9, 2024 07:37
IamPete1
IamPete1 approved these changes May 12, 2024
View reviewed changes
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

@robertlong13 @IamPete1
AP_Motors_test: add tri frames to json output
5318cf8
@robertlong13 @IamPete1
AP_MotorsTri: reverse-frame cleanup and fixes
e7a6bc2 
- fix motor test order for reverse frame
- add frame type string for reverse frame
- fix initialization of _pitch_reversed flag
@IamPete1 IamPete1 force-pushed the pr/ap_motors_json_tri branch from a143b31 to e7a6bc2 Compare May 12, 2024 20:28
@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

@robertlong13 robertlong13 mentioned this pull request May 14, 2024
Merged
@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
Reviewers

@IamPete1 IamPete1 IamPete1 approved these changes

@rmackay9 rmackay9 rmackay9 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@robertlong13 @IamPete1 @tridge @rmackay9 @CraigElder @peterbarker