Skip to content

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

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

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

andrewleech
Copy link
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
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
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
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
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
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

@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Nov 7, 2022
@ma261065
Copy link

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.

RetiredWizard pushed a commit to RetiredWizard/micropython that referenced this pull request Jul 26, 2024
…n-main

Translations update from Hosted Weblate
@andrewleech andrewleech force-pushed the py/ringbuff_expose branch 2 times, most recently from e81a9ba to 45f1b96 Compare August 7, 2024 05:58
@andrewleech
Copy link
Contributor Author

Thanks @dpgeorge I've updated based on your most recent review. Lets see if the overall coverage is improved even more now!

py/objringio.c Outdated
*
* The MIT License (MIT)
*
* Copyright (c) 2019 Jim Mussared
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be your copyright instead, if you authored this entire file yourself?

will allocate a 17 byte buffer internally so it can hold 16 bytes of data.
When passing in a pre-allocated buffer however one byte less than its
original length will be available for storage, eg. ``RingIO(bytearray(16))``
will only hold 15 bytes of data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth mentioning that reading and writing are thread and IRQ safe when used separately?

mp_printf(&mp_plat_print, "%d\n", ringbuf_get_bytes(&ringbuf, get, 7));
mp_printf(&mp_plat_print, "%s\n", get);
// Prefill ringbuffer
for (int i = 0; i < 98; ++i) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 98, maybe sizeof(buf) - 2? Or even sizeof(buf) - 3 to indicate that you allow 2 bytes free space, then the 7 added below will not fit.

// Should fail - too full.
mp_printf(&mp_plat_print, "%d\n", ringbuf_put_bytes(&ringbuf, put, 7));
// Should fail - buffer too big.
uint8_t large[105] = {0};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe sizeof(buf) + 5?

@andrewleech
Copy link
Contributor Author

They're helpful suggestions thanks @dpgeorge, all addressed in latest push.

mp_printf(&mp_plat_print, "%d\n", ringbuf_get_bytes(&ringbuf, get, 7));
mp_printf(&mp_plat_print, "%s\n", get);
// Prefill ringbuffer
for (int i = 0; i < (sizeof(buf) - 3); ++i) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int -> uint8_t, and no parenthesis around the sizeof-3 (for consistency)

@andrewleech andrewleech force-pushed the py/ringbuff_expose branch 2 times, most recently from 260821a to 3c43f98 Compare September 16, 2024 01:40
@andrewleech
Copy link
Contributor Author

Thanks @dpgeorge that last build failure from the coverage addition has been fixed.

This commit adds a new `RingIO` type which exposes the internal ring-buffer
code for general use in Python programs.  It has the stream interface
making it similar to `StringIO` and `BytesIO`, except `RingIO` has a fixed
buffer size and is automatically safe when reads and writes are in
different threads or an IRQ.

This new type is enabled at the "extra features" ROM level.

Signed-off-by: Andrew Leech <andrew.leech@planetinnovation.com.au>
@dpgeorge dpgeorge merged commit 7e14680 into micropython:master Sep 19, 2024
65 of 66 checks passed
@dpgeorge
Copy link
Member

Thanks for updating, now merged!

@AmirHmZz
Copy link
Contributor

Thanks everyone for working on this!

@andrewleech andrewleech deleted the py/ringbuff_expose branch September 24, 2024 01:35
@ma261065
Copy link

ma261065 commented Jan 9, 2025

I got my first chance to use RingIO in a project the other day. I needed a 100kB buffer for some audio data and thought this would be perfect.

However, it seems that the size of the buffer is limited to 64kB by the use of uint16_t in this struct in ringbuf.h:

typedef struct _ringbuf_t {
    uint8_t *buf;
    uint16_t size;
    uint16_t iget;
    uint16_t iput;
} ringbuf_t;

So, I had to change them to uint32_t and compile a build, and then it worked just fine.

Is there a good reason that these were made uint16 and not uint32?

Could it be updated in the next release to allow larger buffers?

@andrewleech
Copy link
Contributor Author

andrewleech commented Jan 9, 2025

The ringbuf C structure is used on some much smaller devices where every byte of ram counts, so it was originally made smaller as there were no large needs.

Maybe a separate larger version of the struct could be defined and used instead for situations where a larger ringio object is created.

Actually that probably will overcomplicate things a bit much. Perhaps the size of the elements used in ringbuf struct could be defined by the ROM device size defines so on larger devices a uint32 is used.

Regardless these changes will need a new PR, could you raise this as a new issue or even submit your uint32 change as a PR?

@ma261065
Copy link

ma261065 commented Jan 9, 2025

Thanks Andrew. I appreciate that we're generally running on resource-constrained devices, and so a good default would have been that "64kB should be enough for anybody".

Maybe I'm missing something, but as far as I understand the only change required is to change those three uint16_t to uint32_t, and this will work on small or large devices. Obviously if you try and allocate a ring buffer bigger than the memory available on the small device it will fail, but I don't see the need to have different versions for different devices.

Am I missing something?

(And, yes, I'll raise an issue as creating a PR is beyond my skills at this point)

@andrewleech
Copy link
Contributor Author

Changing these three elements from uint16 to unit32 is a global change in the firmware so will affect other existing ringbuf usages in C. Each ringbuf instance will then use an additional 6 bytes (2 per element in the struct). This might not seem like much but in the existing usages this is extra ram used for no feature gain.

@ma261065
Copy link

ma261065 commented Jan 9, 2025

Fair enough - but isn't "The ability to create larger ring buffers" a feature gain?

And, unless I'm misunderstanding "affect other existing ringbuf usages" - this wouldn't be a breaking change would it?

Personally, I think it's worth the 6 extra bytes, but I'll defer the decision to you as the author.

@dpgeorge
Copy link
Member

dpgeorge commented Jan 9, 2025

I think the simplest thing to do would be make the uint16_t for the size configurable by the port. And make it uint32_t only if the port has more than 64k of heap available (otherwise there's no point in it being larger).

@ma261065
Copy link

ma261065 commented Jan 9, 2025

Issue raised - #16560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants