Accomplish requisite work to close out PR #11431. #12507

mcsauder · 2019-07-18T02:27:18Z

Describe problem solved by the proposed pull request
This PR simplifies the diff in PR #11431, although it might produce a few merge conflicts for @catch-twenty-two to resolve (apologies). The changes proposed in this PR include alphabetizing #includes and stubbing out method declarations.

This PR is aimed at advancing/closing out PR #11431.

Additional context
See PR #11431.

mhkabir · 2019-07-18T04:03:04Z

@mcsauder Can you just pull the entirety of #11431 into here so that we can merge the complete feature in one go?

julianoes · 2019-07-18T13:05:34Z

@mcsauder I'd suggest that you request review from someone appropriate, otherwise I'm not sure how this should proceed.

mcsauder · 2019-08-14T05:44:00Z

If this doesn't break anything, let's just add it and deal with any repercussions after we discover an issue!

mcsauder · 2019-09-12T02:48:23Z

@dagar, this is pretty low risk, any concerns I can address?

julianoes · 2019-09-20T12:19:12Z

src/modules/simulator/simulator_mavlink.cpp

+				mavlink_play_tune_t tune_msg {};
+				mavlink_message_t message {};
+
+				memcpy(tune_msg.tune, &tone_info, sizeof(tone_info));


Why are you creating the tone_info struct and then copying from it into the mavlink_play_tune_t struct? Couldn't you use the mavlink_play_tune_t struct from the beginning. Also, I wonder if the padding could be different if mavlink_play_tune_t is packed and the struct above is not.

Thanks @julianoes , I think the reason this was done originally is that the two two structs differ in how the get_next_note() method operates. get_next_note() gives back unsigned int values for frequency, duration, silence, and volume, and that 16 bytes of data needs to get packed into the mavlink_play_tune.tune[] 30 byte char array. Is there a better way I could try to implement this? Thanks!

Aha, now I understand. Does the tone_info struct need to be packed then? Otherwise padding might mess with the format, right?

I think you're right about that, but I'm missing something there. When I try to pack that struct it can't bind the values in the get_next_note() call.

Hm, I see. Where is the counterpart for this function? The one sending the tunes?

Or asking differently? How can I test/use this?

FYI @JonasVautherin

Hi @julianoes , the counterpart to this PR is here, now closed. I've tried again to resolve conflicts and so far had only concluded that I don't actually understand everything going on. :/ Give me a little bit of time to try to figure everything out, otherwise, perhaps, we can just close out this PR.

src/modules/simulator/simulator_mavlink.cpp

mcsauder · 2019-10-12T03:32:15Z

Intermingled with this PR is #12309... The two are attempting to utilize the same functionality from different viewpoints... I am appreciative for input from anyone who wrote this code and understands it!

…ded #includes, add volume to send_sim_tone() message and minor formatting.

catch-twenty-two · 2019-10-22T21:02:07Z

Intermingled with this PR is #12309... The two are attempting to utilize the same functionality from different viewpoints... I am appreciative for input from anyone who wrote this code and understands it!

Hi @mcsauder thanks for continuing to work on this. This code only runs in the PX4 simulator, the other issue seems to be on the actual hardware. So this code is never active when the firmware is loaded on an actual hardware platform and executed. Does that make sense?

mcsauder · 2019-10-24T18:06:04Z

Hi @mcsauder thanks for continuing to work on this. This code only runs in the PX4 simulator, the other issue seems to be on the actual hardware. So this code is never active when the firmware is loaded on an actual hardware platform and executed. Does that make sense?

Thanks @catch-twenty-two , I perhaps wasn't all that clear in my reference above. I think that the intent of this PR is actually at odds with what kind of data this message is meant to carry. It looks to me that we should be loading an ACSII music string instead of the struct that is being proposed in this PR and was originally submitted. I think we need to revisit this PR in that light and am closing this work for now since we lack functionality on the simulator side to receive as well.

catch-twenty-two · 2019-10-24T18:21:00Z

Ah got it. In that respect a lot more work would need to be done on the sim side, since it was basically just playing a tone for a specified time and not interpreting a string of characters. I was trying to get it to mimic a bare bones buzzer as closely as possible to reflect exactly how the px4 stack would have played it. I guess a message long would have made more sense, when I get some time I will reopen this with the necessary changes.

…

On Thu, Oct 24, 2019 at 11:06 AM Mark Sauder ***@***.***> wrote: Hi @mcsauder <https://github.com/mcsauder> thanks for continuing to work on this. This code *only* runs in the PX4 simulator, the other issue seems to be on the actual hardware. So this code is never active when the firmware is loaded on an actual hardware platform and executed. Does that make sense? Thanks @catch-twenty-two <https://github.com/catch-twenty-two> , I perhaps wasn't all that clear in my reference above. I think that the intent of this PR is actually at odds with what kind of data this message is meant to carry. It looks to me that we should be loading an ACSII music string instead of the struct that is being proposed in this PR and was originally submitted. I think we need to revisit this PR in that light and am closing this work for now since we lack functionality on the simulator side to receive as well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12507>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRHPO6QBA7MA4G2SQVP733QQHPZRANCNFSM4IEWLOQQ> .

-- *Jimmy Johnson* Field Application Engineer Phone: ‪(253) 242-3689‬ Seattle, Wa www.auterion.com

mcsauder force-pushed the advance_pr_11431 branch from 7431cab to 76ff08d Compare July 18, 2019 02:50

mcsauder force-pushed the advance_pr_11431 branch from 76ff08d to edc4a98 Compare July 19, 2019 00:47

mcsauder changed the title ~~Accomplish some requisite work to close out PR #11431.~~ Accomplish requisite work to close out PR #11431. Jul 19, 2019

mcsauder requested a review from bkueng July 19, 2019 04:42

mcsauder force-pushed the advance_pr_11431 branch from bb0fcfb to 2ac6195 Compare July 19, 2019 05:04

weekly-digest bot mentioned this pull request Jul 21, 2019

Weekly Digest (14 July, 2019 - 21 July, 2019) #12528

Closed

mcsauder force-pushed the advance_pr_11431 branch 3 times, most recently from 71a8afb to abcdbc2 Compare July 25, 2019 22:52

weekly-digest bot mentioned this pull request Jul 28, 2019

Weekly Digest (21 July, 2019 - 28 July, 2019) #12569

Closed

mcsauder force-pushed the advance_pr_11431 branch 4 times, most recently from 683eeae to 5900c06 Compare August 7, 2019 21:44

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #12682

Closed

weekly-digest bot mentioned this pull request Aug 18, 2019

Weekly Digest (11 August, 2019 - 18 August, 2019) #12733

Closed

mcsauder force-pushed the advance_pr_11431 branch from 5900c06 to bdd1fb4 Compare August 19, 2019 03:17

weekly-digest bot mentioned this pull request Aug 25, 2019

Weekly Digest (18 August, 2019 - 25 August, 2019) #12803

Closed

mcsauder force-pushed the advance_pr_11431 branch from bdd1fb4 to 5ce94c8 Compare August 29, 2019 00:16

weekly-digest bot mentioned this pull request Sep 1, 2019

Weekly Digest (25 August, 2019 - 1 September, 2019) #12858

Closed

mcsauder force-pushed the advance_pr_11431 branch from 5ce94c8 to 5c9f543 Compare September 2, 2019 05:09

weekly-digest bot mentioned this pull request Sep 8, 2019

Weekly Digest (1 September, 2019 - 8 September, 2019) #12924

Closed

weekly-digest bot mentioned this pull request Sep 15, 2019

Weekly Digest (8 September, 2019 - 15 September, 2019) #12959

Closed

mcsauder mentioned this pull request Sep 17, 2019

Add mavlink play tune message to simulator class #11431

Closed

julianoes reviewed Sep 20, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Sep 22, 2019

Weekly Digest (15 September, 2019 - 22 September, 2019) #13002

Closed

mcsauder force-pushed the advance_pr_11431 branch from 5c9f543 to 5f3049d Compare October 2, 2019 16:13

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

Weekly Digest (29 September, 2019 - 6 October, 2019) #13096

Closed

julianoes reviewed Oct 7, 2019

View reviewed changes

src/modules/simulator/simulator_mavlink.cpp Outdated Show resolved Hide resolved

mcsauder force-pushed the advance_pr_11431 branch 2 times, most recently from bdc06cd to a7ca36c Compare October 9, 2019 17:50

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

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

Closed

Accomplish the requisite work to close out PR PX4#11431: remove unnee…

369645d

…ded #includes, add volume to send_sim_tone() message and minor formatting.

mcsauder force-pushed the advance_pr_11431 branch from a7ca36c to 369645d Compare October 18, 2019 18:08

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

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

Closed

mcsauder closed this Oct 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accomplish requisite work to close out PR #11431. #12507

Accomplish requisite work to close out PR #11431. #12507

mcsauder commented Jul 18, 2019

mhkabir commented Jul 18, 2019

julianoes commented Jul 18, 2019

mcsauder commented Aug 14, 2019

mcsauder commented Sep 12, 2019

julianoes Sep 20, 2019

mcsauder Oct 2, 2019

julianoes Oct 7, 2019

mcsauder Oct 7, 2019

julianoes Oct 8, 2019

julianoes Oct 10, 2019

julianoes Oct 10, 2019

mcsauder Oct 12, 2019

mcsauder commented Oct 12, 2019

catch-twenty-two commented Oct 22, 2019 •

edited

Loading

mcsauder commented Oct 24, 2019

catch-twenty-two commented Oct 24, 2019 via email

Accomplish requisite work to close out PR #11431. #12507

Accomplish requisite work to close out PR #11431. #12507

Conversation

mcsauder commented Jul 18, 2019

mhkabir commented Jul 18, 2019

julianoes commented Jul 18, 2019

mcsauder commented Aug 14, 2019

mcsauder commented Sep 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcsauder commented Oct 12, 2019

catch-twenty-two commented Oct 22, 2019 • edited Loading

mcsauder commented Oct 24, 2019

catch-twenty-two commented Oct 24, 2019 via email

catch-twenty-two commented Oct 22, 2019 •

edited

Loading