Add crsf-telemetry filtered queues

[wimalopaan]

This PR tries to implement #5757

It introduces the following new lua functions:

• crossfireTelemetryCreatePrivateQueue()
• crossfireTelemetryPopPrivate()
• crossfireTelemetryRemovePrivateQueue()

and it changes (non-breaking) the interface of

• create()

of lua widgets.

For lua widgets that use crossfireTelemetryPop() nothing changes.

The create(zone, options) function gets a third parameter create(zone, options, widget_id). The widget_id is a unique widget-identifier, that will be used in the other three new lua functions.

• crossfireTelemetryCreatePrivateQueue(widget_id, filter_table)

inserts a new filter for the widget with the id widget_id. filter_table can contain up to 8 bytes that are compared against the first 8 bytes (startung with the length byt eof the frame) of an incoming crsf packet. 0 values serve as wild-cards. If a crsf-packet matches it is sorted out into the according private queue.

This function returns error codes:

• -1 if an equivalent filter already exists,

• -2 if no more private filter queues are possible (up to 8 filter queues are possible)

• crossfireTelemetryPopPrivate(widget_id)

extract a crsf message out of the private crsf message queue for widget with widget_id. Return values are the same as for crossfireTelemetryPop().

• crossfireTelemetryRemovePrivateQueue(widget_id)

removes a private crsf message queue (this also happens automatically if the widget is terminated).

[wimalopaan]

Attached is a lua widget to test

crsfqueue.TGZ

[pfeerick]

What happened with the reversion of #5788 in the last commit?

Are you happy with how this is working now?

[wimalopaan]

The last revision 57e0ef2 reverts an eeroneously included change of the DEBUG_BAUDRATE. A change of the DEBUG-BAUDRATE to the standard baudrate of 460800 (for TX16s) was recently add by @3djc via a different PR.

I would like to get some feedback to the change of the LUA create function introduction a third parameter with a widget_id. Maybe there is a better way to create this id?

Well, I would be happy if some people would at least test that this PR does not change the behaviour ofcrossfireTelemetryPop().

[3djc]

I'm lost, at what speed do you want it ? You asked for it to be changed: https://github.com/EdgeTX/edgetx/pull/5788/files, and now you want it back to 400k ?

[wimalopaan]

I just did not want to include a totally unrelated thing in this PR! Your change to 460800 is that what I wanted ;-) 3djc ***@***.***> schrieb am Mo., 3. Feb. 2025, 08:43:

…

I'm lost, at what speed do you want it ? You asked for it to be changed: https://github.com/EdgeTX/edgetx/pull/5788/files, and now you want it back to 400k ? — Reply to this email directly, view it on GitHub <#5773 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCLD74OYU7ERUUIXMGA4O32N4MYTAVCNFSM6AAAAABU67PSYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMZQGE4DSMRQHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[wimalopaan]

I'll do a rebase of this PR to make the things clear. Wilhelm Meier ***@***.***> schrieb am Mo., 3. Feb. 2025, 09:07:

…

I just did not want to include a totally unrelated thing in this PR! Your change to 460800 is that what I wanted ;-) 3djc ***@***.***> schrieb am Mo., 3. Feb. 2025, 08:43: > I'm lost, at what speed do you want it ? You asked for it to be changed: > https://github.com/EdgeTX/edgetx/pull/5788/files, and now you want it > back to 400k ? > > — > Reply to this email directly, view it on GitHub > <#5773 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACCLD74OYU7ERUUIXMGA4O32N4MYTAVCNFSM6AAAAABU67PSYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMZQGE4DSMRQHE> > . > You are receiving this because you authored the thread.Message ID: > ***@***.***> >

[wimalopaan]

Added new test widget version: https://github.com/wimalopaan/LUA/tree/main/WIDGETS/CrsfQueue

[wimalopaan]

Rebased

[wimalopaan]

I would like to get some feedback to the change of the LUA create function introduction a third parameter with a widget_id. Maybe there is a better way to create this id?

Any comments on this? I'm still unsure ...

My ideal solution would be, that the widget code does not have to pass this widget_id as a parameter in the function calls, but that (every / some, if they need it) LUA call transparently passes the widget_id to the function (like an object-reference in OOP). But I had no idea how to implement that ...

[philmoz]

This seems overly complicated to me.

Looking at the changes, this is limited to Color radios and Lua widgets.
So I would suggest something like this:

• Add the luaInputTelemetryFifo pointer to the base Widget class, set to nullptr by default
• When a Lua widget calls crossfireTelemetryPop(), if the widget local copy of luaInputTelemetryFifo is null, create the fifo and store it in the widget class object. Also add it to a global vector of fifo pointers.
• When the widget destructor is called, if luaInputTelemetryFifo is not null, remove it from the global list of fifo pointers and free the allocated memory.
• When you get some telemetry data you want to push, iterate through the global list of fifo pointers and push the new data onto all of them.

This way:

• The fifo is automatically created for any widget that wants telemetry data, and is destroyed when the widget is destroyed.
• All widgets wanting telemetry will see their own copy of all data, if the data is not relevant it is just ignored.
• No changes are needed to the Lua script code.

This should also be extended to the sportTelemetryPop and ghostTelemetryPop functions for consistency.

[wimalopaan]

Thank you for your input! And yes, the filtering scheme maybe unneccessary. The idea was to offload the filtering fro LUA code to native code ... but I think, I'll drop that then.

• When a Lua widget calls crossfireTelemetryPop(), if the widget local copy of luaInputTelemetryFifo is null, create the fifo and store it in the widget class object.

That was my problem: how does crossfireTelemetryPop() find / reference its queue? For this to solve I introduced the widget_id, but I'm not happy with that ...

[philmoz]

That was my problem: how does crossfireTelemetryPop() find / reference its queue? For this to solve I introduced the widget_id, but I'm not happy with that ...

Ah, yes I see the problem now.

There is hack in the code for the lcd.exitFullScreen() function where a pointer to the current LuaWidget object is saved in the 'runningFS' variable.
This might be usable in your case with the following caveats:

• it only applies to widgets, it is not set for stand alone tools
• it is only set for the widget 'refresh' and 'background' calls, it would not work for the getter and settings functions of Lvgl widget controls.

To fix these properly will likely need a more robust solution that the current 'runningFS' variable - I need to think about this some more. I will be away for a few days though.

[wimalopaan]

This might be usable in your case with the following caveats:

• it only applies to widgets, it is not set for stand alone tools
• it is only set for the widget 'refresh' and 'background' calls, it would not work for the getter and settings functions of Lvgl widget controls.

I think this would be too errorprone!

To fix these properly will likely need a more robust solution that the current 'runningFS' variable - I need to think about this some more.

I'm awaiting your input ;-)

[raphaelcoeffic]

@wimalopaan is that designed like a subscribe/notify model? I'm still wondering how requests/replies are related to each other. It seems they aren't, right? So basically you can send some request, and you subscribe the reply type (your filters). Does that reflect the design idea?

Bonus question: I guess this new API does not solve the multiple module issue (internal & external CRSF working at the same time), right?

[raphaelcoeffic]

This should actually return a LUA object. luaCrossfireTelemetryRemovePrivateQueue should probably be declared as __gc for that object table, which would be called when the object is garbage collected.

[raphaelcoeffic]

After reading the code, I also have the feeling we could have something similar with less complexity and less templates. Templates classes are not cheap in terms of compiled code size and compile time. Also, I'm not sure how this is really required here.

To me this looks like a simple subscribe/notify model, where actually LUA scripts should own the buffers, and the telemetry receiver would basically check for subscribers for specific message types, and push the payload to each of these subscribers. I don't really understand why you need the widget_id, and how an ID collision is prevented.

matches [are] sorted out into the according private queue

No requirement for multiple subscribers for a given type of message? Is it assumed that filters for each widgets are disjoint? Or is that a mere copy and the same message could be posted to multiple queues?

I would use a simple array with a max of X subscribers, or a vector if you insist (thus using dynamic re-allocation), however this could all be hidden from the headers, thus avoiding all these template classes in headers, which are actually just private implementation.

And yes, I do second @philmoz that this should be a generic facility that could actually be used by any protocol, and not just CRSF. Each protocol will obviously interpret subscriptions ("filters") their own way, that should be it.

PS: deeply sorry for not reviewing earlier!

[wimalopaan]

to me this looks like a simple subscribe/notify model, where actually LUA scripts should own the buffers,

Yes.

push the payload to each of these subscribers.

dependant on the filter: if the filter matches, the message is put into the private queue for that widget (subscriber).

I don't really understand why you need the widget_id, and how an ID collision is prevented.

Each widget has to use it's own queue, and the widget_id serves as a unique id (technically it is the options-table index for that widget, so no collision possible if I understand that part correct). The queue is destroyed when the widget is destroyed (lua widget d-tor).

No requirement for multiple subscribers for a given type of message? Is it assumed that filters for each widgets are disjoint? Or is that a mere copy and the same message could be posted to multiple queues?

The code ensures that two filters are disjoint: if a widget wants to install a filter that "overlaps" with an already installed filter, it gets an error.

I totally agree now, that this was too complicated (the basic idea was to offload the filtering into native code). And I now agree to your and @philmoz idea, just to provide a private copy of the messages into a private queue for each widget. The widget itself has then to decide if she wants to further process that message.

So, all we need is (replace crossfireTelemetryPop() with all sport..., ... )

1. all incoming messages are distributed to the complete set of private queues (maybe 0 to N)

2. if widget calls crossfireTelemetryPop(), it pops a message from it's private queue. If such a private queue does not exist, it is created.

3. if widget is destroyed, it's private queue gets deleted.

My actual problem in doing so is: how does a crossfireTelemetryPop()call knows which private queue it should use? It needs a reference to the calling widget (like a hidden this pointer in OOP). That was the reason to introduce the widget_id, I never was happy with.
Then the pointer to that private queue could be placed into the widget object so it can be destroyed when the widget gets destroyed.
Well, looks like I did not get the conecpt of how the widget ressources get managed very well.

Sorry, for this badly designed PR ... but obviously I need your help to fix that.
Many thanks!

[philmoz]

To fix these properly will likely need a more robust solution that the current 'runningFS' variable - I need to think about this some more.

I'm awaiting your input ;-)

I think I have a solution to better allow the Lua API functions to access the current script context (widget, or stand alone script). Need to do some more testing.

I also need to think about whether custom mixer scripts and special / global function scripts could also be affected - do they need to call the crossfireTelemetryPop() function?

[wimalopaan]

I also need to think about whether custom mixer scripts and special / global function scripts could also be affected - do they need to call the crossfireTelemetryPop() function?

From a practical point of view I would say: no (never needed that in the past). From an architectural point of view I would say: yes (why impose a restriction on that?).