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_EFI/AP_PiccoloCAN: Add Currawong ECU support #20399

Merged
merged 10 commits into from Sep 20, 2022

Conversation

reilly-callaway
Copy link
Contributor

  • Reads telemetry data over CAN and pass to existing AP_EFI_Backend.
  • Configurable to send throttle commands over CAN to a single ECU.

Example with Currawong ECU connected to CubeOrange over CAN. Shows EFI mavlink status in Mission Planner (matching data in Currawong's internal tool) with throttle set by Ardupilot:

image

Copy link
Member

@hendjoshsr71 hendjoshsr71 left a comment

Choose a reason for hiding this comment

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

Quick comment to get past your first CI error:

Add the following under here should fix it

'kg/m/m' : 'kilograms per square meter', # metre is the SI unit name, meter is the american spelling of it

         'kg/m/m/m'  : 'kilograms per cubic meter',

We will eventually need to clean up you commit list a bit by squashing the "clean up" commits. ArduPilot doesn't use merge commits. Instead we rebase our working branch on top of the master (development) branch. https://ardupilot.org/dev/docs/git-interactive-rebase.html

I can help you with GIT if needed?

Copy link
Member

@hendjoshsr71 hendjoshsr71 left a comment

Choose a reason for hiding this comment

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

Some small changes.

Note: I didn't look over the PiccoloCAN parts yet

libraries/AP_EFI/AP_EFI.cpp Outdated Show resolved Hide resolved
libraries/AP_EFI/AP_EFI.h Outdated Show resolved Hide resolved
libraries/AP_EFI/AP_EFI_Currawong_ECU.h Outdated Show resolved Hide resolved
libraries/AP_EFI/AP_EFI_Currawong_ECU.cpp Outdated Show resolved Hide resolved
libraries/AP_EFI/AP_EFI_Currawong_ECU.cpp Outdated Show resolved Hide resolved
@reilly-callaway
Copy link
Contributor Author

Rebased on top of master and to clean up commit history, happy to squash it down more if you'd like.

Copy link
Member

@hendjoshsr71 hendjoshsr71 left a comment

Choose a reason for hiding this comment

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

The commits look reasonable enough to me.

Some style things. I didn't go through the generated code.

I'm probably not the person for reviewing the CAN changes. As I'm not as familiar with the their structure enough to ensure thread safety.

This one is outside of my scope to approve. But I'm glad to see it built locally now :)

libraries/AP_PiccoloCAN/AP_PiccoloCAN.cpp Outdated Show resolved Hide resolved
libraries/AP_PiccoloCAN/AP_PiccoloCAN.cpp Outdated Show resolved Hide resolved
libraries/AP_PiccoloCAN/AP_PiccoloCAN.cpp Outdated Show resolved Hide resolved
libraries/AP_PiccoloCAN/AP_PiccoloCAN.cpp Outdated Show resolved Hide resolved
libraries/AP_PiccoloCAN/AP_PiccoloCAN.cpp Outdated Show resolved Hide resolved
@reilly-callaway reilly-callaway force-pushed the currawong-ecu branch 2 times, most recently from a3e5093 to f66cc5e Compare March 30, 2022 05:20
@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Mar 30, 2022
@reilly-callaway reilly-callaway force-pushed the currawong-ecu branch 2 times, most recently from ba213a9 to b600ddc Compare March 31, 2022 01:08
@SchrodingersGat
Copy link
Contributor

@hendjoshsr71 any further requirements for review on this?

libraries/AP_EFI/AP_EFI.cpp Outdated Show resolved Hide resolved
libraries/AP_EFI/AP_EFI.cpp Outdated Show resolved Hide resolved
libraries/AP_EFI/AP_EFI_Currawong_ECU.h Show resolved Hide resolved
Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

looks nice!

libraries/AP_EFI/AP_EFI.cpp Outdated Show resolved Hide resolved
@reilly-callaway
Copy link
Contributor Author

@tridge @peterbarker Any updates on having this approved?

@tridge
Copy link
Contributor

tridge commented Jun 22, 2022

@reilly-callaway needs a rebase as conflicted, then ping me to merge this

@reilly-callaway
Copy link
Contributor Author

@tridge Rebased and all tests passing. Should be good to merge.

@peterbarker peterbarker merged commit e4a0ea6 into ArduPilot:master Sep 20, 2022
@peterbarker
Copy link
Contributor

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants