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_Scripting: RGB LED override binding #12892

Merged
merged 4 commits into from Dec 13, 2019
Merged

Conversation

@IamPete1
Copy link
Contributor

IamPete1 commented Nov 26, 2019

This work was funded by 3DXR.

This adds a new method to AP_Notify to allow temporary overriding all other RGB led methods. The binding for this is then added to scripting. Along with a example to blink the lights.

https://www.youtube.com/watch?v=TyiMCSSfkz0&feature=youtu.be

@IamPete1 IamPete1 added the Scripting label Nov 26, 2019
@IamPete1 IamPete1 requested a review from WickedShell Nov 26, 2019
libraries/AP_Notify/RGBLed.h Outdated Show resolved Hide resolved
Copy link
Contributor

tridge left a comment

just a small update needed in header to give units

@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Nov 27, 2019

The question I have is do we want to just have a NTF_LED_OVERRIDE that puts all LED control in the hands of scripting? The time part of the override actually seems a bit odd to me, as it means you have to run a script on a tighter deadline to avoid reversion to old LED colors then just having full control over it. Personally I think the duration just complicates the LED side from scripts but if others want it then I can see it. (With the param stuff it gets quite easy to do temporary script LED control by just changing NTF_LED_OVERRIDE around as needed)

@IamPete1 IamPete1 force-pushed the IamPete1:scripting_led branch from 20097c8 to 07265cd Nov 27, 2019
@IamPete1

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Nov 27, 2019

@WickedShell I actually like the duration better than the param change, Its probability better not to encourage people to change params more than necessary using scripting. I would be tempted to move the mavlink stuff that way too, have the duration as a mavlink extension and default to some reasonable time out, 30s or some thing. The current max duration (if you don't use 0 for for ever) is 65 seconds, not really to onerous of a call back time. May even be worth moving from ms to s for the duration, duration of less than one seconds are probably not that useful. Then we could do 18 hours (or change to a smaller variable)

@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Nov 27, 2019

I'm mostly not sold because if you miss for any reason you get a flash of a different color, if you are using the LED to indicate a safety state from your script that momentary flash could be really disturbing. I guess we could still have a NTF_LED_OVERRIDE mode that says "scripting only" and still have the duration call and when it expires just don't do anything. It just bugs me cause it means larger scripts for the general control case :) But if we want the time based one then I won't block it. Tagging it as a dev call topic incase there isn't a conclusion before then.

@CraigElder CraigElder removed the DevCallTopic label Dec 3, 2019
@IamPete1

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Dec 3, 2019

move to a override parameter with no duration

@IamPete1 IamPete1 force-pushed the IamPete1:scripting_led branch from 07265cd to 72fb4a6 Dec 9, 2019
@IamPete1

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Dec 9, 2019

Moved to not having a override duration and sharing LED_OVERRIDE type 1 with mavlink, tested on CubeOrange with here2

@WickedShell WickedShell merged commit a45041c into ArduPilot:master Dec 13, 2019
4 checks passed
4 checks passed
ArduPilot.ardupilot #20191209.37 succeeded
Details
ArduPilot.ardupilot (Cygwin SITL build) Cygwin SITL build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details
@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Dec 13, 2019

We may want to make a variant for the future that allows rate_hz to be optional, but we might as well bring it in for now.

@IamPete1 IamPete1 deleted the IamPete1:scripting_led branch Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.