ZephyrSerial (and probably others) - proper way to use Semaphores?

[KurtE]

I am wondering if, the ZephyrSerial class is properly using Semaphores.

Background:
As I have mentioned, I have been playing around with zephyr directly and currently playing with some
of the Teensy boards. I have been trying out some of the sample sketches and now trying to extend some.
Like the USB sketch the echoed data to itself trying to convert it to a USB to Serial adapter sketch/project...
Not sure what they call them in zephyr.

So I started off, just trying to add ability to output a string directly to the USB Port.

void usb_print_string(const char *msg) {
	uart_fifo_fill(usb_uart_dev, (const uint8_t *)msg, strlen(msg));
}

Then read that uart_fifo_fill can only be called from isr callback... So then figured needed to add a ring buffer
... And soon I found I was more or less duplicating the ZephyrSerial class...

Issue:

The ISR code does not check the return code from k_sem_take,
For example, if some code is in:

size_t arduino::ZephyrSerial::write(const uint8_t *buffer, size_t size)
{
	int idx = 0;

	while (1) {
		k_sem_take(&tx.sem, K_FOREVER);
		auto ret = ring_buf_put(&tx.ringbuf, &buffer[idx], size-idx);
		k_sem_give(&tx.sem);
		if (ret < 0) {
			return 0;
		}

When the hardware decides to ask for more data (either empty, or not full, or watermark).
The callback function is called: arduino::ZephyrSerial::IrqHandler()
The processing of the TX code looks like:

	k_sem_take(&tx.sem, K_NO_WAIT);

	if (ring_buf_size_get(&tx.ringbuf) == 0) {
		uart_irq_tx_disable(uart);
	}

	while (uart_irq_tx_ready(uart) && ((length = ring_buf_size_get(&tx.ringbuf)) > 0)) {
		length = min(sizeof(buf), static_cast<size_t>(length));
		ring_buf_peek(&tx.ringbuf, &buf[0], length);

		ret = uart_fifo_fill(uart, &buf[0], length);
		if (ret < 0) {
			break;
		} else {
			ring_buf_get(&tx.ringbuf, &buf[0], ret);
		}
	}
	k_sem_give(&tx.sem);

So what happens if the current running code is in the middle of a write and taken the semaphore?
The ISR is then called, who tries to take it, without waiting. It does not look at the return value
and blindly uses the tx.ringbuf and then does a k_sem_give, where again it did not sucessfully
take it.

So should the above code, look something like:

	if (k_sem_take(&tx.sem, K_NO_WAIT) == 0) {

	    if (ring_buf_size_get(&tx.ringbuf) == 0) {
		    uart_irq_tx_disable(uart);
	    }

	    while (uart_irq_tx_ready(uart) && ((length = ring_buf_size_get(&tx.ringbuf)) > 0)) {
		length = min(sizeof(buf), static_cast<size_t>(length));
		ring_buf_peek(&tx.ringbuf, &buf[0], length);

		ret = uart_fifo_fill(uart, &buf[0], length);
		if (ret < 0) {
			break;
		} else {
			ring_buf_get(&tx.ringbuf, &buf[0], ret);
		}
	    }
	    k_sem_give(&tx.sem);
        }

But then what happens? That is the reason for the ISR callback has not been satisfied, and I know with
some systems, returning from the ISR it will see that and immediately call the ISR again. So
the one holding the Semaphore may likely get starved and we maybe hang?

Should this ISR code, then detect that it did not grab the semaphore, and then disable the TX interrupt,
knowing that the one who currently owns it, will then reenable the Interrupt after it puts some data
into the ring buffer?

The RX side has similar issue.

Or potentially maybe the ISR does not need to try to take the semaphore. As if it is the only producer in the RX one or consumer in the TX one, the two sides might be safe from each other.

As the ringbuffer documentation says:

For the trivial case of one producer and one consumer, concurrency control shouldn’t be needed.

Thoughts?