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: add param set and get #12889

Open
wants to merge 6 commits into
base: master
from

Conversation

@IamPete1
Copy link
Contributor

IamPete1 commented Nov 25, 2019

This work was funded by 3DXR.

This adds the ability to get and set param values from scripts. I have purposely omitted param set_and_save. So any changes made to param's will be lost over a reboot. Param get is harmless, Param set has massive power so should be used with care.

I have also added a example script that uses the two new features.

@IamPete1 IamPete1 added the Scripting label Nov 25, 2019
@IamPete1 IamPete1 requested a review from WickedShell Nov 25, 2019
@magicrub

This comment has been minimized.

Copy link
Contributor

magicrub commented Nov 25, 2019

why is saving params bad? You can do it over GCS easily enough. Is the issue about changing them too fast/often?

@IamPete1

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Nov 25, 2019

My thoughts were more that we probably don't want to let scripts make any permanent param changes. Programming mistakes could lead to bad things, although I guess probably not very much worse than setting a param bad in the first place. I could be convinced to add set_and_save.

Copy link
Contributor

WickedShell left a comment

Params are a case where our internal API is just a badly named fit for exposing to the scripts, a script should probably only have 2 main interfaces:

  • `get("FOO") to get the float value
  • `set("FOO", 1.0) to set the value of FOO and save it (if changed)

We may later decide we want a path which doesn't auto save, but we should be prioritizing the normal case, which is to probably save the changes. We may need to make the set() a non generated API call to support this. (You could yank the generated code then just tweak the order in which it calls methods though)

/*
get a value by name
*/
float AP_Param::get_by_name(const char *name)

This comment has been minimized.

Copy link
@WickedShell

WickedShell Nov 25, 2019

Contributor

Return a bool, and do the float as a reference. This lets the script do nil punning, and allows you to distinguish between not finding the parameter, and a value of 0.

This comment has been minimized.

Copy link
@WickedShell

WickedShell Nov 25, 2019

Contributor

Also any reason you can't rename this to just get(const char *) ?

This comment has been minimized.

Copy link
@IamPete1

IamPete1 Nov 29, 2019

Author Contributor

I have changed this to return bool, cant say I 100% understand how it works in lua but I copied how the battery amps function works and my test script works as expected. get_by_name makes scene in the context of ap param, would be nice to use simplified function names for lua, I can see how to change the lua name in the generated code, but no idea how to change the generator to allow a different name to be given in the .desc

This comment has been minimized.

Copy link
@WickedShell

WickedShell Dec 2, 2019

Contributor

So whats happening here is the binding generator (because of the Null tag) is saying that any values that are annotated with it will only be valid if the function returns true. If it returns true then the function will push those values onto the lua stack so that the function can return them, if it's false then it will simply push a single nil value onto the stack. The main feature of this is it prevents you from forgetting to check the return type and using a random floating point value off the stack as if it was valid. (If you just blindly used the return type you'd eventually get an arithmetic error in most places, or a type error depending on what you did).

This comment has been minimized.

Copy link
@IamPete1

IamPete1 Dec 2, 2019

Author Contributor

Thanks for the explanation

@rmackay9

This comment has been minimized.

Copy link
Contributor

rmackay9 commented Nov 25, 2019

I think it makes sense not to save the parameters or at least have both options ("set" and "set_and_save"). I think set-and-save momentarily slows down the main loop... although perhaps lua runs in a separate thread so it won't cause that delay.

I guess we don't need to worry about thread safety?

BTW, this is a great feature. This will be super useful.

@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Nov 25, 2019

Sets are actually pushed to a worker thread so there isn't a delay problem. There is however a race condition with the save queue with the vehicle code. We currently get away with it because everything is pushed from the same thread, this introduces a second thread which you are correct, that's unsafe and needs to be addressed before this can come in.

@IamPete1 IamPete1 force-pushed the IamPete1:scripting_pram branch from 563405b to a33ea43 Nov 29, 2019
@IamPete1 IamPete1 force-pushed the IamPete1:scripting_pram branch from a33ea43 to b9fded1 Nov 29, 2019
@IamPete1

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Nov 29, 2019

I have added a set_and_save binding, not really sure where to start on making this thread safe, maybe it would suffice to say 'don't use this and set the same prams via GCS' for the time being?

@rmackay9

This comment has been minimized.

Copy link
Contributor

rmackay9 commented Nov 29, 2019

@IamPete1, the param saving queue race condition is apparently related to hidden parameters (I found out about it while writing this wiki page).

It seems that many parameter's index will change when one of the hidden parameters is made visible. When something requests a parameter to change, the request for the change goes into a queue. This means that if there are say 5 parameter requests queued up and the first one to get executed changes one of the hidden parameters, the remaining 4 requests could be executed using invalid indexes.

I think it's too much to ask @IamPete1 to resolve this. I've added he devcalltopic so we can discuss.

@WickedShell

This comment has been minimized.

Copy link
Contributor

WickedShell commented Dec 2, 2019

I'd really prefer to see the function in scripting look like:

param:get("NTF_LED_OVERRIDE")

As this would be less verbose, and a better name then get_name(). I don't think it would give us any problems internally to name it to that (indeed even in the main code base if we reuse the helper I think it would actually read better).

@IamPete1

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Dec 3, 2019

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

This comment has been minimized.

Copy link
Contributor Author

IamPete1 commented Dec 3, 2019

or look at a custom version of the ObjectBuffer - ObjectBufferWithSemaphore

https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_HAL/utility/RingBuffer.h

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.