IPC code @dhylands @fdesre #5
Conversation
There was a problem hiding this comment.
This feels wrong to me since it make things synchronous. I normally have a list of free buffers, and the receiver would just put the buffer back onto the list when he's done with it. Then things stay more or less async until you run out of buffers, in which case the producer will block waiting for a buffer to be freed.
| if (res < 0) { | ||
| return -1; | ||
| } | ||
| return Print(sizeof(buf), buf); |
There was a problem hiding this comment.
For printing, I think its a waste to have a printing task, especially since this is synchronous. I really like the way that the linux kernel printk works in that it can be called from within an ISR or at thread level. This print function can only be called from thread level.
There was a problem hiding this comment.
I also have a implementation of printf over here:
https://github.com/dhylands/projects/blob/master/common/StrPrintf.c#L189
which takes a pointer to a function that outputs a single char. This version doesn't have floating point support, so its quite a bit smaller that the regular sprintf code. Because it takes a pointer to a function you don't need to allocate stack buffers to hold the intermediate results.
There was a problem hiding this comment.
Skimming over your print code, there doesn't seem to be concurrency control.
Would you consider re-licensing the code under MPL 2?
There was a problem hiding this comment.
The print task is not really for Print, but for output over the serial port. My idea was that in the future, there can be a logger task that maintains a small, static-size log of error messages on the device. Release builds would use the logger to record the last ~100 errors and possibly report them to use.
Using a task provides the necessary concurrency control via the involved IPC. FreeRTOS comes with these *FromISR() functions. We could add one for print as well (i.e., PrintFromISR) that prints directly, but maybe interferes with the regular output.
There was a problem hiding this comment.
Yeah - the StrXPrintf code doesn't have any concurrency in it, since it doesn't really make sense to have any in it. Concurrency needs to be dealt with outside of the formatting.
Some users might want to acquire a lock, call StrXPrintf and pass in a function which writes to the serial port. Some users might just want to call StrXPrintf and have the output function do the necessary mutual exclusion (I tend to prefer the former). Functions like sprintf don't do any concurrency control either.
The big advantage of using StrXPrintf is that you don't need to create a temporary buffer to format the output into. You just provide a function which takes characters and outputs them to your LCD or serial port, or whatever.
The license at the top of StrPrintf.c already states that the code is considered public domain, so you can do with it as you wish.
|
I think what you really want is a consumer wait queue with more than 1 element. This way a producer can put its message into the queue even if there's contention. Besides that, I think the code already does want you want. Producers can send as many messages as they want at the same time, and come back later to check for the results: IPCMessage msg1, msg2; For completely async operation, set the NOWAIT flag on the message and there will be no reply from the consumer returned to the producer. The source of the buffer is up to you. If you want to allocate from a list of buffers, you're free to do so. Originally, I had a struct IPCMessageProducer that maintained the buffer. But I found that it's not required at all. We can add this interface on top of the current patch set, if you prefer buffer lists. Be aware that shared buffer lists between producers and consumers have contention as well. And this is a form of dynamic allocation with all the problems it has (e.g., contention, starvation, OOMs, leaks). |
|
Oh! Wanting to fix the consumer's queue length, I saw that the queue already has up to 10 elements. [1] That should be more than enough for our use cases. Since you thought this code is only for synchronous communication, does it help if I add a short tutorial to explain how to use the IPC? [1] https://github.com/tdz/sensorweb-firmware/blob/ipc/src/IPCQueue.c#L163 |
… message The length of the IPC message buffer is stored as part of a larger value. The new helper function extracts the length.
IPC message can optionally transfer buffers from the producer to the consumer. These buffers are not copied while the message is in transit. Consequently, the consumer must inform the producer when it has finished processing the buffer. This patch adds a monitor variable that the producer uses to wait for the competion of the consumers message handling. Once done, the consumer informs the producer to continue. The reply can also return a buffer back to the consumer. The lifetime of this buffer is managed by a higher-level protocol.
The consumer of a message will reply to the producer when it has finished processing a message. This additional IPC can be avoided by setting the flag NOWAIT on the outbound message. Both, consumer and producer, will act as if the reply has been send implicitly.
|
I pushed an update to this PR. It fixes two bugs in the VPrint buffer handling and IPC-message cleanup. Most of all it adds 'raw' output functions like PrintFromISR() that work from within ISRs. If you're OK with re-licensing the your string function under MPL 2, I'd like to use them for the implementation of Print, et al. That would allow for removing the VPrintFromISR-internal buffer from the ISR stack. I also pushed a branch named 'terminal' [1] that implements input over the serial port and a simple command-line interface. That will become my next PR after the basic IPC landed. |
|
I was probably reading more into things from the comments, rather than the code. I think that an example would probably be good anyways. |
Instead of initializing the serial port from the serial-out task, it's safer to initialize the serial port from the main task before doing any related work.
…al port The new functions SerialPutChar() and SerialPutString() output charcters directly to the serial port. This is useful for debugging or in ISRs, where IPC is not available. Don't use them in regular code.
...from dhylands/projects@8572079 The imported code is in the Public Domain.
This patch provides a header file for the StrPrint functions and integrates the source file into the build.
The functions PrintFromISR() and VPrintFromISR() perform formatted output from ISRs. Don't use them outside of an ISR. The functions do not synchronize with the output of the regular print functions of any task.
|
I added tutorial-style documentation to the IPC code base and integrated the StrPrint helper functions. One good point about the latter is the reduction of the text-segment size by ~16 KiB. |
|
The content of the PR is completely out of date. I suggest to simply close the PR. |
No description provided.