-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
Allow scripting to override notify displays #15857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a time out for the scripting notify LED stuff. In the end we went to a param for that. I guess we could do the same here for consistency. Or just have a bool that is flipped on the first message and a scripting function to relinquish control.
@@ -27,6 +27,9 @@ class NotifyDevice { | |||
// give RGB and flash rate, used with scripting | |||
virtual void rgb_control(uint8_t r, uint8_t g, uint8_t b, uint8_t rate_hz) {} | |||
|
|||
// Allows scripting to override the display message | |||
virtual void scr_disp_overide(uint8_t r, const char *str) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtual void scr_disp_overide(uint8_t r, const char *str) {} | |
virtual void scr_disp_overide(uint8_t line, const char *str) {} |
should make it a little clearer.
void update_scr_screen(void); | ||
|
||
// stop scripting override if we have not recieved anything for 5 sec | ||
static const uint16_t _script_timeout_ms = 5000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be a #define, or just hard code it with a comment if its only used in one spot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static const is generally better then #define
void Display::scr_disp_overide(uint8_t r, const char *str) { | ||
// Prevent chars being left from previous message by clearing with spaces first | ||
memset(_scr_msg[r], ' ', DISPLAY_MESSAGE_SIZE-1); // leave null termination | ||
strncpy(_scr_msg[r], str, strlen(str)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to check that str is shorter than DISPLAY_MESSAGE_SIZE-1
// Allow scripting to override text lines on the display | ||
void Display::scr_disp_overide(uint8_t r, const char *str) { | ||
// Prevent chars being left from previous message by clearing with spaces first | ||
memset(_scr_msg[r], ' ', DISPLAY_MESSAGE_SIZE-1); // leave null termination |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just memset to 0?
memset(_scr_msg[r], ' ', DISPLAY_MESSAGE_SIZE-1); // leave null termination | ||
strncpy(_scr_msg[r], str, strlen(str)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memset(_scr_msg[r], ' ', DISPLAY_MESSAGE_SIZE-1); // leave null termination | |
strncpy(_scr_msg[r], str, strlen(str)); | |
strncpy(_scr_msg[r], str, sizeof(_scr_msg[r]) - 1); |
The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null
byte among the first n bytes of src, the string placed in dest will not be null-terminated.
If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n
bytes are written.
@@ -127,6 +127,7 @@ include AP_Notify/AP_Notify.h | |||
singleton AP_Notify alias notify | |||
singleton AP_Notify method play_tune void string | |||
singleton AP_Notify method handle_rgb void uint8_t 0 UINT8_MAX uint8_t 0 UINT8_MAX uint8_t 0 UINT8_MAX uint8_t 0 UINT8_MAX | |||
singleton AP_Notify method handle_scr_disp void uint8_t 0 5 string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad name for the method from a script side. Something like set_display_text()
maybe?
@@ -377,6 +377,17 @@ void Display::update() | |||
} | |||
timer = 0; | |||
|
|||
// If we have received an override from scripting recently allow update | |||
if (AP_HAL::millis() - _last_scr_override <= _script_timeout_ms) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this block the display for the first 5 seconds after boot? You probably need a boolean for "we have overrides" and then once outside the time window set the boolean back to false.
@Gone4Dirt what is the status of this? |
Needs a tidy up. I think there is value in scripting having access to the I2C screens. Will dust it off and get this in. |
I think you need either a lock, or some extra work to ensure that the buffer doesn't get corrupted due to being changed by the scripting one then being stepped on by a normal call to update() could result in some very odd effects on some of the calls like draw_text that is looping over the buffer. As an aside we should probably consider adding a limiter to the loop in draw_text, because a unbounded while loop like this is a bit sketchy. |
Closed by #26066 |
This PR enables scripting to override the little I2C displays in notify. Using scripting to do this adds a lot more flexibility to the use case of these screens.
If a script crashes the override will timeout after 5 secs. Then normal notify behaviour will be resumed.
An example script is provided that allows the screen to be used as an onboard battery checker, for pre-flight checks for example. The outputs from the example script are:
Another example use case (spoilers of things to come ;-) )