lib: bm_fifo: a FIFO queue implementation

[lemrey]

A simple FIFO queue implementation using a circular buffer.

Unsure what error codes to use so I went with errno, but I can change that.

[eivindj-nordic]

Quick look at API. I don't think we should need both this and #155.

[lemrey]

The main purpose of #155 is to ensure we can bring in the mcumgr subsystem, which depends on it.
The queues are quite different, this one copies the data, it is static (implemented as a circular buffer), and thread safe.
The other is list based and dynamic (allocates nodes), it does not copy the data, and it is not thread safe.

[eivindj-nordic]

The main purpose of #155 is to ensure we can bring in the mcumgr subsystem, which depends on it. The queues are quite different, this one copies the data, it is static (implemented as a circular buffer), and thread safe. The other is list based and dynamic (allocates nodes), it does not copy the data, and it is not thread safe.

Ok, with thread safe I assume you mean safe to use in interrupt context etc.. Though, is there a reason we need to have both queue implementations in the repository? Can this for example be rewritten in a way that enables the mcumgr (with some simple glue if required) to use it? That would clean up a bit. Edit: Looks from Jamies response no, so perhaps we should add both, then revisit down the road.

[lemrey]

I have renamed this to bm_fifo to implement a FIFO.

[carlescufi]

Do not use errno.h error codes, use NRF_* ones as discussed and agreed.

[carlescufi]

You are reinventing the wheel? nrfx already has this and it's portable to all Architectures.

[carlescufi]

See NRFX_CRITICAL_SECTION_ENTER, which you will have to define in nrfx_glue.h anyway.

[lemrey]

It's a good a idea to re-use the nrfx macro for this, but that won't work as it implemented currently by the nrfx glue in Zephyr. The issue is that irq_lock()/arch_irq_lock() don't mask zero latency interrupts which what the SoftDevice IRQs are defined as. That's probably correct in Zephyr but here we need to mask those.

If we wanted to re-use nrfx macro we would have to implement our own glue here, and "de-glue" Zephyr somehow, correct @nika-nordic ?

[nika-nordic]

if nrfx_glue.h is taken from NCS, then NRFX macros are implemented as irq_lock/unlock https://github.com/nrfconnect/sdk-zephyr/blob/main/modules/hal_nordic/nrfx/nrfx_glue.h#L129 . Not sure if we can override them differently than with crude #undef -> #define

[MirkoCovizzi]

There seems to be some confusion on this topic. Zephyr does not disable priority 0 interrupts when using irq_lock and irq_unlock which are also used by nrfx_glue.

This behavior can be tested in practice by entering a critical section and executing a long operation while the device is connected via BLE. __disable_irq will disable ALL interrupts indiscriminately and cause an immediate disconnect (timeout) due to an impact on the time-sensitive operations of RADIO0 (and other PRIO_HIGH) interrupts. No critical section should be allowed to affect SoftDevice strict timing requirements. irq_lock will not cause such behavior.

Zephyr, internally, uses the intrinsics for the BASEPRI register and only disables interrupts above a certain priority level (by default above priority 0). Priority 0 is not disabled. Hence, we can safely use irq_lock and irq_unlock in Bare Metal, and so the nrfx_glue abstraction layer. We don't need a custom implementation of critical section logic when using the SoftDevice.

[nika-nordic]

docs states that https://github.com/zephyrproject-rtos/zephyr/blob/f4a0beb2b7b15162fa272117f6fbd76b7c76b292/doc/hardware/arch/arm_cortex_m.rst?plain=1#L293

By modifying BASEPRI (or BASEPRI_MAX) arch_irq_lock() masks all system and HW
interrupts with the exception of

• SVCs
• processor faults
• ZLIs

This allows zero latency interrupts to be triggered inside OS critical sections.

So the SD does not use ZLI then?

[MirkoCovizzi]

@nika-nordic SoftDevice, for some (but not all) internal operations (e.g. radio), uses zero latency interrupts and priority 0. And it's ok for these to be triggered inside our application's critical sections, because those interrupt callbacks happen inside the SoftDevice and will not be exposed to the user (and to the application). In summary, when SoftDevice is present, priority 0 should never be disabled.

[eivindj-nordic]

@nrfconnect/ncs-co-build-system Please have a look.

[bjarki-andreasen]

It looks like the bm_fifo here is near identical to zephyr's ring_buffer which supports items, while not using any kernel primitives . Maybe just wrap that?

[eivindj-nordic]

It looks like the bm_fifo here is near identical to zephyr's ring_buffer which supports items, while not using any kernel primitives . Maybe just wrap that?

For now I think we should continue with this as is, as other work depends on it. As the API remains the same, we can swap the implementation later if we see that as beneficial.

[bjarki-andreasen]

It looks like the bm_fifo here is near identical to zephyr's ring_buffer which supports items, while not using any kernel primitives . Maybe just wrap that?

For now I think we should continue with this as is, as other work depends on it. As the API remains the same, we can swap the implementation later if we see that as beneficial.

I don't get that, bm_fifo is not implemented yet, and even if so would be used very sparingly at this time, why not just use the well tested zephyr implementation from the get-go?

[MirkoCovizzi]

It looks like the bm_fifo here is near identical to zephyr's ring_buffer which supports items, while not using any kernel primitives . Maybe just wrap that?

For now I think we should continue with this as is, as other work depends on it. As the API remains the same, we can swap the implementation later if we see that as beneficial.

I don't get that, bm_fifo is not implemented yet, and even if so would be used very sparingly at this time, why not just use the well tested zephyr implementation from the get-go?

Right now this library is being used by #161. We recognize that wrapping Zephyr’s ring_buffer is an option worth considering in the future, and wrap it as @eivindj-nordic suggests, but for now we are proceeding with this implementation to avoid blocking ongoing work.

We welcome discussion and criticism of components in active development. If you’re interested, you’re welcome to join the weekly NCS Bare Metal Sync meetings where these and other decisions are discussed and motivated. Please let me know if that is of interest to you and I will arrange accordingly.

[bjarki-andreasen]

It looks like the bm_fifo here is near identical to zephyr's ring_buffer which supports items, while not using any kernel primitives . Maybe just wrap that?

For now I think we should continue with this as is, as other work depends on it. As the API remains the same, we can swap the implementation later if we see that as beneficial.

I don't get that, bm_fifo is not implemented yet, and even if so would be used very sparingly at this time, why not just use the well tested zephyr implementation from the get-go?

Right now this library is being used by #161, which is being implemented by an external contributor. We recognize that wrapping Zephyr’s ring_buffer is an option worth considering in the future, and wrap it as @eivindj-nordic suggests, but for now we are proceeding with this implementation to avoid blocking ongoing work.

ok, I will talk with riadh, then probably unblock this PR based on his feedback :)

We welcome discussion and criticism of components in active development. If you’re interested, you’re welcome to join the weekly NCS Bare Metal Sync meetings where these and other decisions are discussed and motivated. Please let me know if that is of interest to you and I will arrange accordingly.

Please add me to the meetings yes :) its usually faster to talk than comment :)

[carlescufi]

Right now this library is being used by #161, which is being implemented by an external contributor. We recognize that wrapping Zephyr’s ring_buffer is an option worth considering in the future, and wrap it as @eivindj-nordic suggests, but for now we are proceeding with this implementation to avoid blocking ongoing work.

Note that Riadh is not an external contributor, he's part of our team. Regarding this, I have to agree with @bjarki-andreasen. Zephyr's ring buffer in item mode covers the exact same functionality, it's very widely tested, it is partially thread-safe and it requires absolutely no Zephyr kernel primitives at all. It really is duplicating code for the sake of it?

While I don't find this direction the most efficient, work has already been done, so I'm not going to block this one