Ringbuf: fix PacketBuffer; clean up ringbuf implementation and use

[dhalbert]

• ringbuf usage in PacketBuffer formerly would wrap around and overwrite an older packet on overflow, including its length field, which would cause a bad packet. Now an overflow simply drops the excess packet, preserving the older packets. (This is the bug that this PR is fixing.)

• In various UART implementations, ringbuf used to overwrite the oldest data with newer data if there was an overflow. Now excess new characters are simply dropped. Per @jepler, this is how Linux works, and is a better idea. This is a change in the semantics. But the old way was not documented, so I don't think we have to worry too much about changing this.

• ringbuf used two indices to keep track of the last written and last read positions. In this kind of implementation, the capacity of an n byte buffer is n-1. The calling routines didn't know this, so they were actually allocating an n-1 ring buffer when they though they were allocating n slots. For UART this was not so critical, but for PacketBuffer it made the ringbuf buffer too small.

• ringbuf formerly was implemented solely as a set of inline routines and a macro in ringbuf.h. ringbuf.c was added, and the inlines in ringbuf.h were made regular C routines. The allocation macro was turned into a routine. We can rely on the compiler to do the inlining as needed.

• ringbuf was used as a very leaky abstraction in a number of places. Various routines reached into its struct. New routines were added to add needed functionality, and all the calling code was fixed to respect the abstraction.

• Struct fields named rbuf were renamed to ringbuf for clarity.

• The ringbuf routines to read and write multiple bytes could be used in more places, and now are.

• The m.iMX implementation used ringbuf to allocation a simple buffer. But i.MX has its own ringbuf routines, so using ringbuf was unnecessary, and was changed to a simple alloc.

• ports/nrf/mpconfigport.mk was not providing defaults for some build options. @jepler: take a look here. I didn't know about ?= in makefiles; we could probably simplify a bunch of ifdef code later by using ?=, though I have not done that right now.

• PacketBuffer.packet_size was rewritten to return min(packet size, mtu-3) in all cases. When there is no connection, the default MTU size is used. @tannewt: take a look at this especially.

[dhalbert]

The single subjob build failure is bogus: it's a job failure fetching dependencies.

[tannewt]

I think packet_size needs to be asymmetric because WRITES can be as long as the attribute length while NOTIFY is limited by the MTU. Based on the doc here it needs to be the larger of the two: https://github.com/adafruit/circuitpython/blob/master/shared-bindings/_bleio/PacketBuffer.c#L100

We'll want to validate that outgoing data fits into the mtu if NOTIFY is being used.

Overall, a very nice cleanup! Just a couple details to sort out.

[jepler]

I have a few notes, but I think they're mostly because I lacked the context or (in one case) talking about a preexisting shortcoming that is not affected by this PR.

[dhalbert]

I think packet_size needs to be asymmetric because WRITES can be as long as the attribute length while NOTIFY is limited by the MTU. Based on the doc here it needs to be the larger of the two: https://github.com/adafruit/circuitpython/blob/master/shared-bindings/_bleio/PacketBuffer.c#L100

We'll want to validate that outgoing data fits into the mtu if NOTIFY is being used.

@tannewt I am adding Connection.max_packet_length, which returns the maximum number of bytes that will fit in a single transmitted packet (MTU size - 3). It will throw an exception if you call it before the connection is established, since we don't know the MTU size.

Packet_buffer.packet_size could call this, but it will need to check if its characteristic is NOTIFY or not. And now we'll need to defer creating the local bytearray buffers in the various libraries (MIDI, etc.), until we have a connection. We can just set them to None in __init__ and create them on demand.

The logic for computing .packet_size is simple. Assuming we have Connection.max_packet_length as describe above, it becomes a convenience method only. It could be dropped and the user code could decide whether to use Characteristic.max_length or Connection.max_packet_length, based on the user code knowing about NOTIFY or not.

In either case we will need to update the libraries that use PacketBuffer.packet_size.

[tannewt]

@tannewt I am adding Connection.max_packet_length, which returns the maximum number of bytes that will fit in a single transmitted packet (MTU size - 3). It will throw an exception if you call it before the connection is established, since we don't know the MTU size.

I don't think you will be able to raise an exception because a Connection object doesn't exist prior to it being established.

How will you know the maximum amount of bytes you can read out? If you are a server with a write and notify characteristic, then a client could write more than the MTU to you even though you can only write an MTU back.

[dhalbert]

I don't think you will be able to raise an exception because a Connection object doesn't exist prior to it being established.

Right, sorry, I was calling bleio_connection_ensure_connected(), which will throw an exception if the connection is no longer connected. So PacketBuffer.max_packet_length will throw an exception if it's not connected, because it doesn't know the MTU and so can't answer reliably.

How will you know the maximum amount of bytes you can read out? If you are a server with a write and notify characteristic, then a client could write more than the MTU to you even though you can only write an MTU back.

That's a bit pathological (WRITE/NOTIFY vs the typical READ/NOTIFY), but I"m thinking the point of .max_packet_length is how big a notify packet the server can send, and the point of PacketBuffer is to queue notify packets. Maybe we should just call it max_notify_packet_length or similar, to be more precise.

[tannewt]

How will you know the maximum amount of bytes you can read out? If you are a server with a write and notify characteristic, then a client could write more than the MTU to you even though you can only write an MTU back.

That's a bit pathological (WRITE/NOTIFY vs the typical READ/NOTIFY), but I"m thinking the point of .max_packet_length is how big a notify packet the server can send, and the point of PacketBuffer is to queue notify packets. Maybe we should just call it max_notify_packet_length or similar, to be more precise.

It's not pathological. That's how USB MIDI works.

[dhalbert]

It's not pathological. That's how USB MIDI works.

All right, I stand corrected. So I'm thinking that what one wants .packet_size to reflect varies based on which direction you're interested in and what the flags are. Perhaps the best thing is to drop it, and have the user code figure out the packet length required based on the situation, the new Connection.max_packet_length, and on the Characteristic's max_length. Or do you think calling it .max_notify_length makes more sense?

[tannewt]

How about renaming it incoming_packet_size but have it manage the notify vs write difference?

[dhalbert]

@tannewt OK, I think this is finally ready for review again. I decided not to rename .packet_size, but it would have create churn in the libraries, and I decided the alternate names were not really clearer. The doc is clearer.

This has been tested with BLE MIDI both out and in, with ble_uart_echo_test.py, and with some other libraries that use PacketBuffer.

Also, taking a cue from @jepler (thanks!), I changed a bunch of makefile ifndefs to use ?=, which makes them a lot shorter.

[tannewt]

A couple questions about packet_size. Thanks for taking this on.

[tannewt]

A few minor comments. Should be good to go shortly.

[tannewt]

Thanks for all of these fixes!

[dhalbert]

Whew, thanks! A lot of technical debt paid off, at the cost of too much time.