Skip to content

DroneCAN: support sending/receiving DroneCAN messages in lua #29310

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

Merged
merged 4 commits into from
May 9, 2025

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Feb 17, 2025

This adds support for sending and receiving DroneCAN messages from lua scripts.

It includes an example script that subscribes to messages, sends requests and sends broadcast messages

Note that I have not done a lua compiler for DSDL, so manual message parsing is needed

This PR relies on this libcanard PR: dronecan/libcanard#74
and this DSDL update: dronecan/DSDL#67

Due to the simplification of libcanard this PR also saves about 4k of flash

should I move the GlobalTime broadcast to C++? Depends if other devices could use it

@tridge tridge force-pushed the pr-dronecan-param-lua branch from d4e5afe to ebfa1e5 Compare February 17, 2025 05:27
@tridge tridge force-pushed the pr-dronecan-param-lua branch from ebfa1e5 to 60a1e4e Compare April 18, 2025 22:22
@tridge tridge force-pushed the pr-dronecan-param-lua branch 6 times, most recently from a3ada15 to 7096e3f Compare April 20, 2025 07:00
@tridge tridge force-pushed the pr-dronecan-param-lua branch 2 times, most recently from c1904d7 to 0bfefcd Compare April 21, 2025 01:55
@tpwrules
Copy link
Contributor

I have to admit I don't understand what getting the internal libcanard semaphore accomplishes. Is it just to avoid deadlocks? I don't think that can happen here.

@tridge tridge force-pushed the pr-dronecan-param-lua branch from 0bfefcd to fa05e8a Compare April 25, 2025 01:58
@tpwrules
Copy link
Contributor

tpwrules commented Apr 25, 2025

the reason we need the get_semaphore() is mutex ordering. If we don't et that mutex before we get the sem in the scripting code then we deadlock if a message arrives during deletion of a Subscriber in DroneCAN_Handle::check_message(). It happens in SITL if you comment out the get_semaphore() code, DroneCAN thread is in handle_message(), scripting in check_message()

Ah yes, I see that now. But why don't we just release h->sem before the delete? The function which sets up h->subscriber does not even take h->sem. It also runs on the same thread as the function doing the delete so I don't think there is a race condition. If we received a message it would just get stored before being thrown away in the destructor.

This also applies to the destroy_all method, I don't see the danger in letting handlers continue to execute while the subscribers are destroyed. The canard lock will protect things.

@tridge tridge force-pushed the pr-dronecan-param-lua branch 3 times, most recently from cbcd22b to fb330e8 Compare May 1, 2025 20:49
@tridge tridge force-pushed the pr-dronecan-param-lua branch 2 times, most recently from bad480a to b38d9c7 Compare May 1, 2025 22:54
@tridge
Copy link
Contributor Author

tridge commented May 1, 2025

@tpwrules I've removed the libcanard get_semaphore() and the per-handle semaphore, replacing it with get_sem_rx() in AP_DroneCAN

@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label May 1, 2025
Copy link
Contributor

@tpwrules tpwrules left a comment

Choose a reason for hiding this comment

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

Happy to see the semaphores cleaned up. I think the per-handler semaphore would have worked fine, but using the DroneCAN one probably saves memory (at the cost of CPU and interference risk).

I definitely would want to see the battery tag changes pulled out in a separate PR.

@tridge tridge force-pushed the pr-dronecan-param-lua branch 2 times, most recently from d03255e to b5b36f4 Compare May 2, 2025 21:40
@tridge tridge force-pushed the pr-dronecan-param-lua branch from b5b36f4 to 205eb34 Compare May 5, 2025 21:37
@tridge
Copy link
Contributor Author

tridge commented May 5, 2025

@tpwrules i've moved the BatteryTag changes into #29983

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.

Scripting stuff looks good, just a small thing in the docs.

Copy link
Contributor

@tpwrules tpwrules left a comment

Choose a reason for hiding this comment

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

Some error condition handling but overall looks good.

@tridge tridge force-pushed the pr-dronecan-param-lua branch 2 times, most recently from bfc73d4 to 2882056 Compare May 6, 2025 04:12
@tridge tridge requested a review from tpwrules May 6, 2025 21:43
@tridge tridge force-pushed the pr-dronecan-param-lua branch from 2882056 to 42c8214 Compare May 6, 2025 23:15
tridge added 4 commits May 8, 2025 13:10
added get_semaphore() and allow for multiple handlers for an ID
allow for sending broadcast messages, subscribing to broadcast
messages and sending requests
@tridge tridge force-pushed the pr-dronecan-param-lua branch from 42c8214 to 43f4c27 Compare May 8, 2025 03:10
Copy link
Contributor

@tpwrules tpwrules left a comment

Choose a reason for hiding this comment

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

The little test example works well on Cube Orange. Great work! I also did some futzing with the REPL and could not immediately break the autopilot or my node.

Future expansion ideas:

  • Autotest
  • Ability to have autopilot itself/other scripts see script-sent messages
  • DroneCAN server support (your idea)
  • Priority handler/inhibit mechanism so a script can eat messages
  • C++-assisted encoding and decoding

@tridge tridge merged commit 78d42fc into ArduPilot:master May 9, 2025
107 of 108 checks passed
@tridge
Copy link
Contributor Author

tridge commented May 9, 2025

@tpwrules thanks for your help with this PR!

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

Successfully merging this pull request may close these issues.

5 participants