Add optional priority attribute for messages

[podhrmic]

The attribute can be used for QoS if the transport layer implements so.

Default priority is 1, higher number has higher priority.

I added dummy put_priority() functions in pprz_transport.c for now.

I did some tests with priority queues, and it works as expected.

[podhrmic]

@gautierhattenberger what do you think?

[flixr]

probably easier to write priority = attrs.get('priority', 1)

[flixr]

Should the default really be 1? Is that the lowest, or is 0 also allowed? What are the min and max priorities?

How are the priorities actually handled?

[podhrmic]

Default can be anything really, 1 is a good number, because 0 indicates "priority not set" which can be useful to know (e.g. when handling errors).

It is a priority byte so 0-255

They are not handled in any way right now (the transport layer doesn't care). I used a priority queue for example to test it over serial link, but other ways are possible too.

[gautierhattenberger]

Obviously, all transports should implement the new put_priority function, which is not yet the case. Also, as you mentioned, the transport layer doesn't care about priority, it is just doing the encapsulation. So it means that you also need to modify the device layer to pass the priority information, implement it in all devices on the paparazzi side and eventually add your priority queue as an option one of them to make it work. Am I right ?

[podhrmic]

The use case I had in mind was to have the priority queue a part of the transport layer. See #70 - the queue is a part of the user code, not directly in pprzlink (I would like to avoid changing the device layer.). OpenDDS (see #69) has a bunch of settings for Quality Of Service, so the priority could nicely fit in there.

So it would mean just adding empty put_priority stubs for the other transports.

[flixr]

For me the question also is if this priority attribute is "only" on a per message type basis (according to messages.xml) and thus fixed at compilation time or one can set a prio for a message when sending it...

[podhrmic]

The "default" priority is set at compilation time, but it can be changed on runtime if needed (see https://github.com/paparazzi/pprzlink/pull/70/files#diff-88c9c4ad71adde0ca53ec6beafc8750fR51 in #70 ), assuming the message handling code implements it.

Although I can't think of a good use case that cannot be done with the default priorities - do you have suggestions?

I think I need to add a priority field into the transport struct, so it can be passed with spprz_transport. Or do you want this to be a part of every transport?

[podhrmic]

@gautierhattenberger any thoughts on this? This could be nicely used in user-defined transport (similar to paparazzi/paparazzi@0e552b4 )

[gautierhattenberger]

I still have one extra concern about the best place to set this priority level.

In your current implementation, it is done after the check_available_space and start functions. Which means that you have probably already started to allocate or reserved buffer space and even started to fill data when you know what is the priority level. I think that to make actual implementation easier, this should be done at an early stage, at least before the start function. We could also consider that it can be an argument of the check_space, as in some implementation, it is used to return the allocated memory through the FD parameter. We already have this kind of things for ChibiOS arch to send and log messages in multi-threaded environment (although not yet priority queues).

[podhrmic]

I like it to be a part of check_space - probably just adding a single argument will be sufficient. Will modify accordingly.

[podhrmic]

Imagine this use case - there is a priority queue that arranges messages based on their priority and sends them at specific intervals. If the queue is full, but your new message has higher priority than the lowest priority message in the queue, the lowest priority message gets dropped, and the new high priority message will be added.

Specifically:

1. check_space(..., msg_priority)tells me if I can add this new message
2. if so, start_message(..., msg_priority) or put_priority(..., msg_priority) then sets the priority for the given message so it can be used later.

So the takeaway - priority should be checked before starting the message with check_space(), and then should be inserted with put_priority before start_message. Does that sound reasonable?

And on a side note - how do you handle mutexes in ChibiOS logging?

[gautierhattenberger]

I would say that if the check_space returns that you have enough space, it should also reserve the space somehow, otherwise you may have memory violation in multi-thread. I'm not sure having the priority indication in two places is really needed, but I guess it all depend of the underlying implementation. I know you plan to do this at transport level. I a previous design (a long time ago...) it was planed to be in device. Not sure if it makes a real difference.
So, to answer your question, yes to point 1, hard to tell for point 2 without an actual code or complete spec behind it.

For ChibiOS, it is actually the device level with the check_free_space function that is locking the device (see for uart https://github.com/paparazzi/paparazzi/blob/master/sw/airborne/arch/chibios/mcu_periph/uart_arch.c#L949) and it is unlocked with the send_message. This way, the access to the uart device is not interrupted while sending a message.
Also, note the use of the fd parameter to tell the driver not to unlock the mutex before the end of the message. This fd is set by check_free_space and passed to the other functions afterward.

[podhrmic]

OK, I ll update the check_free_space and make a simple example (probably in a similar fashion as the crypto transport in paparazzi/paparazzi#2205 )