mavlink: add missing uORB publication of tunes #13158

julianoes · 2019-10-11T10:29:00Z

The old tune device interface is not working anymore and we need to publish to uORB tune_control.

~~This solution is not optimal though because blocks the receiving thread.~~

julianoes · 2019-10-11T11:06:52Z

In my opinion we should change the uorb tunes interface from individual notes to the string and then do the parsing and playing in the the respective tone driver. This way the uorb publisher doesn't need to wait.

Alternatively, we could increase the queue size by a lot but I'm not sure what the implications of that would be.

@bkueng do you have an opinion on that?

bkueng · 2019-10-11T11:57:50Z

@bkueng do you have an opinion on that?

Probably one of these:

add a separate uORB message containing a string
add a string tunes handler class, used in mavlink that does the parsing non-blocking and is regularly called from the receiver thread (most efficient solution)

julianoes · 2019-10-11T14:07:30Z

add a separate uORB message containing a string

What's your concern with changing the existing one instead?

add a string tunes handler class, used in mavlink that does the parsing non-blocking and is regularly called from the receiver thread (most efficient solution)

That's an idea. Do you think it would actually run fast enough though?

bkueng · 2019-10-11T15:12:08Z

What's your concern with changing the existing one instead?

Unnecessary overhead as you need worst-case string length.

That's an idea. Do you think it would actually run fast enough though?

You can try. I'm not sure how accurate the timing really needs to be.

dagar · 2019-10-11T15:24:14Z

At a glance I've questioned the per note orb interface, but it hasn't quite bothered me enough to do anything about it.

I think an orb message containing the string would be fine (with a realistic length). Unless or until we know someone actually cares about doing this over mavlink we could ignore the tune2[200] extension field. https://mavlink.io/en/messages/common.html#PLAY_TUNE

krisklau · 2019-10-16T07:47:43Z

Hi!

Thanks for this, we just hit this ourselves.

Correct me if I have misunderstood the ToneAlarm driver, but could it be an idea to as a first step only support the pre-specified tones on the mavlink interface using the tone_id field? Then the mavlink receiver would only need to forward the tone_id specified in the mavlink message (possibly similar to here: https://github.com/PX4/Firmware/blob/45a53726d6594f6285243c1f1198fa431958e1c5/src/modules/events/rc_loss_alarm.cpp#L100)

Then we can figure out the custom string-based tone later.

krisklau · 2019-10-16T08:28:57Z

Ah, well I see now that the mavlink message does not have a tune_id field. Could repurpose the first bytes in the tune array, but that seems like a bit of a hack.

krisklau · 2019-10-16T09:03:25Z

@julianoes I incorporated the changes I thought about here: krisklau@0ae1ec3

Tested and works on a Pixhawk 4. Do you think this is a viable approach? Should I open a seperate PR for this?

JonasVautherin · 2019-10-16T13:51:43Z

Ah, well I see now that the mavlink message does not have a tune_id field. Could repurpose the first bytes in the tune array, but that seems like a bit of a hack.

This sounds super custom to me, as I basically have no clue about the pre-specified tones on a MAVLink vehicle. I'm already not a big fan of the PLAY_TUNE specs that explicitly states that this is not a standard message 😅:

tune | char[30] | tune in board specific format

krisklau · 2019-10-16T15:47:42Z

hehe yeah I agree, but at least to be able to play something is better than nothing!

For PX4, the list is here:

https://github.com/PX4/Firmware/blob/bfab544a64e2dc3b86905771017989502ec40dd2/src/lib/tunes/tune_definition.desc#L91

krisklau · 2019-10-16T15:48:45Z

Our use-case is primarily just to confirm user actions from a companion computer.

mcsauder · 2019-10-31T17:37:05Z

src/modules/mavlink/mavlink_receiver.cpp

+			tune_control.frequency = (uint16_t)frequency;
+			tune_control.duration = (uint32_t)duration;
+			tune_control.silence = (uint32_t)silence;
+			tune_control.volume = (uint8_t)volume;


Hi @julianoes , would you want to use static_cast<type>(var) , (C++ style casts), here? (It allows compile time type checking where C-style casting does not.)

mcsauder

Hi @julianoes , I think this is a step in the right direction and good for now. The discussion above is all relevant and important, but perhaps another restructuring is in order after this step is complete. Bench testing checks out. I vote to get this in.

julianoes · 2019-11-02T09:53:08Z

@mcsauder Thanks for reviewing and testing. I guess it's only stalling the MAVLink receive thread and nothing else.

While I'm at it here, I should also add PLAY_TUNE_V2 though. I'll do that next week.

julianoes · 2020-01-30T15:43:29Z

Fixes #13158.

src/modules/mavlink/mavlink_receiver.cpp

The old tune device interface is not working anymore and we need to publish to uORB tune_control. This solution is not optimal though because blocks the receiving thread.

Instead of blocking the receiver thread while playing a tune we now copy the tune to a buffer and check if we can play the next note on each iteration of the receiver thread. The buffer and tune object is only created on the heap if we receive a tune to play once and doesn't use resources otherwise.

bkueng · 2020-04-27T09:18:16Z

src/modules/mavlink/mavlink_receiver.h

+	static constexpr unsigned MAX_TUNE_LEN {248};
+
+	// Allocated if needed.
+	Tunes *_tunes {nullptr};


What about moving these into a separate struct/class? Then you'd only have one allocation.

@bkueng done inc8c690d4da86122ac5d58b11eca8fa659d2110b8.

This makes it easier to allocate in MavlinkReceiver.

Mentioned issue is addressed.

julianoes requested review from dagar and mcsauder October 11, 2019 10:29

julianoes force-pushed the fix-mavlink-tunes branch from fd78f79 to 70b1610 Compare October 11, 2019 11:11

weekly-digest bot mentioned this pull request Oct 13, 2019

Weekly Digest (6 October, 2019 - 13 October, 2019) #13171

Closed

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #13236

Closed

mcsauder reviewed Oct 31, 2019

View reviewed changes

mcsauder previously approved these changes Oct 31, 2019

View reviewed changes

julianoes dismissed mcsauder’s stale review via b8837e7 November 4, 2019 10:39

julianoes force-pushed the fix-mavlink-tunes branch from 70b1610 to b8837e7 Compare November 4, 2019 10:39

julianoes mentioned this pull request Jan 30, 2020

Mavlink PLAY_TUNE message does not play tune #12309

Closed

LorenzMeier previously requested changes Jan 30, 2020

View reviewed changes

src/modules/mavlink/mavlink_receiver.cpp Outdated Show resolved Hide resolved

weekly-digest bot mentioned this pull request Feb 2, 2020

Weekly Digest (26 January, 2020 - 2 February, 2020) #14084

Closed

julianoes mentioned this pull request Apr 10, 2020

Is tune example working? mavlink/MAVSDK#1035

Closed

julianoes force-pushed the fix-mavlink-tunes branch from b8837e7 to 4418ea5 Compare April 24, 2020 13:44

julianoes requested a review from bkueng April 24, 2020 13:45

julianoes marked this pull request as ready for review April 24, 2020 13:45

julianoes requested a review from LorenzMeier April 24, 2020 13:47

weekly-digest bot mentioned this pull request Apr 26, 2020

Weekly Digest (19 April, 2020 - 26 April, 2020) #14761

Closed

julianoes added 5 commits April 27, 2020 09:38

mavlink: add missing uORB publication of tunes

64debc2

The old tune device interface is not working anymore and we need to publish to uORB tune_control. This solution is not optimal though because blocks the receiving thread.

mavlink: use static_cast instead of C-style cast

1438232

mavlink: move tune publication to a method

5e749a3

mavlink: add support for PLAY_TUNE_V2

eacdd36

julianoes force-pushed the fix-mavlink-tunes branch from 4418ea5 to 0704870 Compare April 27, 2020 07:39

julianoes requested a review from mcsauder April 27, 2020 07:39

bkueng reviewed Apr 27, 2020

View reviewed changes

mavlink: move tune publication to separate class

c8c690d

This makes it easier to allocate in MavlinkReceiver.

bkueng approved these changes Apr 28, 2020

View reviewed changes

bkueng merged commit 48c60d3 into master Apr 28, 2020

bkueng deleted the fix-mavlink-tunes branch April 28, 2020 06:45

weekly-digest bot mentioned this pull request May 3, 2020

Weekly Digest (26 April, 2020 - 3 May, 2020) #14814

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mavlink: add missing uORB publication of tunes #13158

mavlink: add missing uORB publication of tunes #13158

julianoes commented Oct 11, 2019 •

edited

Loading

julianoes commented Oct 11, 2019

bkueng commented Oct 11, 2019

julianoes commented Oct 11, 2019

bkueng commented Oct 11, 2019

dagar commented Oct 11, 2019

krisklau commented Oct 16, 2019

krisklau commented Oct 16, 2019

krisklau commented Oct 16, 2019

JonasVautherin commented Oct 16, 2019

krisklau commented Oct 16, 2019

krisklau commented Oct 16, 2019

mcsauder Oct 31, 2019

mcsauder left a comment

julianoes commented Nov 2, 2019

julianoes commented Jan 30, 2020

bkueng Apr 27, 2020

julianoes Apr 27, 2020

mavlink: add missing uORB publication of tunes #13158

mavlink: add missing uORB publication of tunes #13158

Conversation

julianoes commented Oct 11, 2019 • edited Loading

julianoes commented Oct 11, 2019

bkueng commented Oct 11, 2019

julianoes commented Oct 11, 2019

bkueng commented Oct 11, 2019

dagar commented Oct 11, 2019

krisklau commented Oct 16, 2019

krisklau commented Oct 16, 2019

krisklau commented Oct 16, 2019

JonasVautherin commented Oct 16, 2019

krisklau commented Oct 16, 2019

krisklau commented Oct 16, 2019

mcsauder Oct 31, 2019

Choose a reason for hiding this comment

mcsauder left a comment

Choose a reason for hiding this comment

julianoes commented Nov 2, 2019

julianoes commented Jan 30, 2020

bkueng Apr 27, 2020

Choose a reason for hiding this comment

julianoes Apr 27, 2020

Choose a reason for hiding this comment

julianoes commented Oct 11, 2019 •

edited

Loading