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_PiccoloCAN: Remove duplicated code #17971

Merged
merged 1 commit into from
Feb 24, 2024
Merged

AP_PiccoloCAN: Remove duplicated code #17971

merged 1 commit into from
Feb 24, 2024

Conversation

amilcarlucas
Copy link
Contributor

@amilcarlucas amilcarlucas commented Jul 8, 2021 •
edited
Loading 

Binary Name      Text [B]     Data [B]     BSS (B)      Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  -----------  -----------  -----------  ----------------------------  -------------------------
ardurover        0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      336516
blimp            0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      620952
arducopter       0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      192324
arduplane        0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      200432
ardusub          0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      397880
antennatracker   0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      643812
arducopter-heli  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      186124

@peterbarker
Copy link
Contributor

This one is conflicting badly now, but still seems to be relevant - the duplicate code is still present in PiccoloCAN. We've had some recent activity in that directory - maybe we can drum up some interest.

@SchrodingersGat - this PR removes some code from PiccoloCAN by leveraging the ESC Telemetry library, and there's a few advantages in doing that - chiefly "less code is good code"

@SchrodingersGat
Copy link
Contributor

@peterbarker do you refer to removing the send_esc_telemetry_mavlink function? It appears it is not being used anywhere. I'm happy to submit a PR which removes that function. Anything else which needs clean-up?

@amilcarlucas
Copy link
Contributor Author

Rebased, please review again.

@amilcarlucas amilcarlucas force-pushed the pr/piccolo_use_AP_ESC_Telem branch 2 times, most recently from 69c7a60 to 8056237 Compare February 13, 2024 18:07
@magicrub
Copy link
Contributor

What's with all the ! and <! stuff?

@amilcarlucas
Copy link
Contributor Author

Half the code had Doxygen comments, half did not.
Now all code has doxygen comments.

@amilcarlucas
Copy link
Contributor Author

@SchrodingersGat can you take it from here?

@amilcarlucas
Copy link
Contributor Author

@SchrodingersGat is this going to bitrot again?

@SchrodingersGat
Copy link
Contributor

@amilcarlucas I'll check it out

@amilcarlucas
Copy link
Contributor Author

This will probably conflict with #26252 so you better do it before that one gets merged.

reilly-callaway
reilly-callaway suggested changes Feb 21, 2024
View reviewed changes
libraries/AP_PiccoloCAN/AP_PiccoloCAN_Servo.cpp Outdated
Comment on lines 30 to 35
if (decodeServo_StatusAPacketStructure(&frame, &status.statusA)) {
newTelemetry = true;
} else if (decodeServo_StatusBPacketStructure(&frame, &status.statusB)) {
AP_ESC_Telem_Backend::TelemetryData t {
.voltage = float(servo.statusB.voltage) * 0.01f,
.current = float(servo.statusB.current) * 0.01f,
.consumption_mah = 0.0f,
.usage_s = 0,
.last_update_ms = 0,
.temperature_cdeg = int16_t(servo.statusB.temperature * 100),
};

update_telem_data(addr, t,
AP_ESC_Telem_Backend::TelemetryType::CURRENT
| AP_ESC_Telem_Backend::TelemetryType::VOLTAGE
| AP_ESC_Telem_Backend::TelemetryType::TEMPERATURE);
newTelemetry = true;
} else if (decodeServo_FirmwarePacketStructure(&frame, &settings.firmware)) {
} else if (decodeServo_AddressPacketStructure(&frame, &settings.address)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this addition is breaking the compile currently.

This change shouldn't be needed as:

  • Servo data is already being logged in AP_PiccoloCAN.cpp
  • Currently other CAN servos (DroneCAN, etc.) aren't reporting voltage/current/temp/etc. over the ESC telem backend for mavlink anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

@SchrodingersGat
Copy link
Contributor

@amilcarlucas we have reviewed and tested locally (after fixing compiling issues). With the changes suggested by @reilly-callaway above, we would be happy with this.

@amilcarlucas
AP_PiccoloCAN: Remove duplicated code
10c7b02 
Fix doxygen markup for consistency
Fix typos
@amilcarlucas
Copy link
Contributor Author

Please approve so that this can get merged once the tests pass.

@amilcarlucas
Copy link
Contributor Author

@peterbarker are you OK with the current changes?

@amilcarlucas
Copy link
Contributor Author

Can someone re-trigger the failing sub test?

@peterbarker peterbarker merged commit 957b05e into ArduPilot:master Feb 24, 2024
92 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks!

@SchrodingersGat
Copy link
Contributor

@amilcarlucas thanks for the work on this

@amilcarlucas amilcarlucas deleted the pr/piccolo_use_AP_ESC_Telem branch February 26, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reilly-callaway reilly-callaway reilly-callaway requested changes

Assignees
No one assigned
Labels
AllVehicles Driver Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amilcarlucas @peterbarker @SchrodingersGat @magicrub @reilly-callaway