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

py/ringbuf: Add micropython.ringbuffer() interface for general use. #9458

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andrewleech
Copy link
Sponsor Contributor

I've had a number of desirable use cases for a python ringbuffer over the years, usually relating to logging and/or connecting streams together in various ways.

This PR adds an optional python interface to the existing C ringbuffer library.


This work was funded by Planet Innovation.

@robert-hh
Copy link
Contributor

robert-hh commented Sep 29, 2022

Looks interesting. What is the difference/advantage over StringIO?

@andrewleech
Copy link
Sponsor Contributor Author

Ummm, I feel a bit like you've exposed a lack of mental agility on my part here @robert-hh I feel like I've never quite equated the two being basically there same!

In an embarrassingly similar vein I raised this just today #9456 but never thought of using this new ringbuffer API on an existing buffer as being the same thing... I even had an earlier version of this PR where a bytearray could be passed in for reuse but couldn't quite think of a good reason to use it. Maybe I'm multitasking too much lately.

Longer term, I've previously had offline discussions about a semi-persistent logging buffer in ram that isn't explicitly wiped at startup (would require linker script updates to do this) such that logging buffered to it could be retrieved after a watchdog reset etc. I was thinking this API may be needed there too... I think the entire ringbuffer object would need to be statically placed in this fixed location in C to also maintain the head/tail idx variables.

Back to your question though, I don't think StringIO/BytesIO have a mechanism to provide a fixed length buffer. if you try to write more than they currently have space for it'll automatically re-allocate a larger buffer and copy the contents. This can be undesirable when buffering larger amounts of data on smaller parts, or when speed is needed, or buffering out of interrupts etc.

It doesn't look like cpython has a way to use BytesIO with fixed length either.

@peterhinch
Copy link
Contributor

This could be very useful. Key properties of a ringbuf are:

  • Inherent thread safety.
  • Guaranteed non-allocating writes.

These allow writing in a hard ISR and reading in the main code (and vice versa).

@andrewleech andrewleech force-pushed the py/ringbuff_expose branch 4 times, most recently from 9efbb49 to 60d6a6a Compare September 30, 2022 05:20
@stephanelsmith
Copy link
Sponsor Contributor

This could be very useful. Key properties of a ringbuf are:

  • Inherent thread safety.
  • Guaranteed non-allocating writes.

These allow writing in a hard ISR and reading in the main code (and vice versa).

Love this idea

@glenn20
Copy link
Contributor

glenn20 commented Oct 2, 2022

Back to your question though, I don't think StringIO/BytesIO have a mechanism to provide a fixed length buffer. if you try to write more than they currently have space for it'll automatically re-allocate a larger buffer and copy the contents. This can be undesirable when buffering larger amounts of data on smaller parts, or when speed is needed, or buffering out of interrupts etc.

Besides the fixed length issue, as I understand it, StringIO/BytesIO don't provide support for the desired ring buffer mode of operation.

@andrewleech
Copy link
Sponsor Contributor Author

StringIO/BytesIO don't provide support for the desired ring buffer mode of operation

Yes of course. The other day I was thinking this PR has a limitation in that a write cannot overwrite unread data, which is sometimes desirable (only buffer latest X bytes of incoming data).

The real benefit of ringbuffer is that the writer has a different tell() to the reader so it's arguably more like a socket pair than a single BytesIO which read/write at the same location.

@peterhinch
Copy link
Contributor

If a write can overwrite unread data I think this would compromise thread-safety.

@glenn20
Copy link
Contributor

glenn20 commented Oct 2, 2022

If a write can overwrite unread data I think this would compromise thread-safety.

The underlying ringbuf_t structure is inherently threadsafe for the specific case where a single thread (or ISR) is always writing and another is reading. It is used for this purpose in the bluetooth module.

This is very useful for my particular use case: writing data in C space from an ISR (or high priority wifi task) and consuming it in python user space.

It is not threadsafe in the general case.

@peterhinch
Copy link
Contributor

@glenn20 I should have clarified that this is the sense in which I meant it. The ISR use case you cite is widespread and important.

If, as @andrewleech proposed, a mode is supported where the write pointer can overwrite unread data, in that mode the limited thread safety is lost.

@andrewleech
Copy link
Sponsor Contributor Author

If a mode is supported where the write pointer can overwrite unread data, in that mode the limited thread safety is lost.

To be clear the underlying C ringbuffer implementation doesn't support this overwrite usage, nor do I have any plans to add it. It's just something that some people think of with generic ringbuffers.

@glenn20
Copy link
Contributor

glenn20 commented Oct 2, 2022

@andrewleech : FYI - I have some work in progress which add a few extra features which are helpful for my use case, including:

  • Add ringbuf_read(r, data, size) and ringbuf_write(r, data, size) which copy data to/from the ringbuffer with memcpy(), rather than byte or word at a time (I want to maximise throughput).
  • Add ringbuf_read_wait(r, data, size, timeout_ms) and ringbuf_write_wait(r, data, size, timeout_ms) which provide C api to blocking IO with timeout.
  • Add mp_obj_new_ringbuffer(buf, size, timeout_ms) to provide C api to creating new ringbuffer objects.
  • Add buffer protocol support for direct access to the underlying buffer.

You can see the changes at: glenn20@158df92 and glenn20@10fc526

If you have an interest in folding any of these into your PR, let me know.

I have this all working in a dev version of the espnow module, where the ringbuffer object is created within my module and exposed to the users as an attribute. The ringbuffer is populated with incoming data from a high priority wifi task on the esp32 and readout from the buffer can now be entirely handled in python code - allowing me to significantly reduce the C footprint of the module and support flexible user-controlled readout strategies.

Thanks for your very timely publication of this PR ;).

@andrewleech
Copy link
Sponsor Contributor Author

Thanks @glenn20 I had been toying with the idea of adding functions to read/write in chunks rather than byte-at-a-time but figured if I did that I'd need to do some benchmarks to check it really was a lot faster and didn't feel like doing the work :-)

I had wondered about making it buffer api compatible but thought it was less important if a buffer was passed in as it'd already be available for access - but adding this interface is important for one created in C, or convenient for one created by length.

That being said I'm still unsure about the utility of directly accessing the buffer in many cases, as the data in generally wouldn't make sense once it had written around then end of the ring.
If the ringuffer is instead being used as a stream read/write once to a buffer ie https://github.com/orgs/micropython/discussions/9456 then this makes sense. For this usage I've added a reset() function to the python class to zero out both iput/iget.

I was also thinking I'd like to make the ringbuffer code compatible with the power-of-2 variant of rungbuffers where you don't lose a byte of buffer to tracking. Was thinking it could auto-select the algorith used depending on whether the supplied buffer meets this criteria. This would make extra accessor functions more complicated though as they'd also have to support both algorithms, so again I'll need to think if the benefits are worth the (code/flash) costs.

It's also worth thinking/talking about whether it'd be better to be blocking or not by default; timeout==0 ot timeout==MAX

@ma261065
Copy link
Sponsor

The ringbuffer is populated with incoming data from a high priority wifi task on the esp32 and readout from the buffer
can now be entirely handled in python code - allowing me to significantly reduce the C footprint of the module
and support flexible user-controlled readout strategies.

Do you happen to have a link to some code where you set up a high-priority wifi task to populate a buffer? This is something that I need to do in a project of mine, so it would be very useful not to have to reinvent the wheel. Much appreciated if you could share...

@glenn20
Copy link
Contributor

glenn20 commented May 18, 2023

The ringbuffer is populated with incoming data from a high priority wifi task on the esp32 and readout from the buffer
can now be entirely handled in python code - allowing me to significantly reduce the C footprint of the module
and support flexible user-controlled readout strategies.

Do you happen to have a link to some code where you set up a high-priority wifi task to populate a buffer? This is something that I need to do in a project of mine, so it would be very useful not to have to reinvent the wheel. Much appreciated if you could share...

Sorry - I didn't setup the high priority task. On the ESP32, the wifi handling code is managed with a set of high-priority threads. Espressif's ESP-NOW api includes a callback when a message is received. The callback I provide is called from one of the wifi high-priority tasks. It is a pradigm that is very similar to running the callback from an ISR.

@AmirHmZz
Copy link
Contributor

AmirHmZz commented Dec 2, 2023

Any updates on this PR? I think ringbuf could be very useful for many projects. Since ringbuf is already implemented in C, this should not increase the code size significantly.

@projectgus
Copy link
Contributor

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants