Studio RPC Infrastructure

[petejohanson]

Initial PR for the RPC infrastructure to be used by ZMK Studio.

This includes the behavior metadata commit from #2231 as it's used by the first really fleshed out behavior subsystem.

To Do

• Document framing and protocol buffer messages.
• Wrap up the sample client library.
• Testing/validation from others.

[and-elf]

Why not start with a cli tool, that handles the actual rpc calls, and layer studio gui ontop of it later?
I'm not sure the gui (library) should be a part of this repo.
The binary could be used by https://github.com/nickcoutsos/keymap-editor

I'm not a maintainer, but it seems to me that the software isn't a part of the firmware.
When I do RPC, I usually keep protocol definitions in an own repo, which server and client references.

[joelspadin]

Checks like this with unsigned integers always make me nervous, but the math above does appear to guarantee that copied <= len.

[petejohanson]

I've tweaked it to the simpler copied < len

[joelspadin]

If len = 0, then this would skip doing anything and exit the loop (if len can be 0, would a while loop be better than do-while?), but is it possible for len > 0 and claim_len = 0, which would result in an infinite loop?

[petejohanson]

So... It will only infinitely look if there's no consumer pulling out of the ring buffer. I will tweak this to be a regular while loop though to be safe.

[joelspadin]

Is this a compiler extension? GCC and Clang seem to allow it, but MSVC fails to compile it, and I wasn't aware it was possible to declare an array of dynamic size on the stack in C.

[joelspadin]

do-while works, but a for loop might be more conventional? Can also just use an int for the counter so you don't have to explicitly avoid it going negative.

[petejohanson]

Switched to an int... I personally favor do/while style for "retries" like this.

[joelspadin]

What does the return value here represent? I assume it's whether the byte should be used as data, but would be nice to document that.

[petejohanson]

Added some basic docs here.

[joelspadin]

Does this just busy loop until it gets a claim? If so, I think this would need to be running on a thread with a lower priority than whatever is consuming the data, or it may deadlock by never allowing that thread to run?

[petejohanson]

A good question... The zmk_rpc_rx_notify will give on the underlying semaphore, but this is in an ISR context for the UART callback, IIRC, so we can't yield... I'm honestly not sure if we're hitting this (yet) in practice, and if so, how it's functioning. Let me dig a bit.

[petejohanson]

Ugh... Ok, so... I think the answer is: We need to size the RX ring buffer large enough to avoid this happening. From https://docs.zephyrproject.org/3.5.0/doxygen/html/group__uart__interrupt.html#gab10942076ac47ecff29e924098049398:

That means that once uart_irq_rx_ready() is detected, uart_fifo_read() must be called until it reads all available data in the FIFO (i.e. until it returns less data than was requested).

So... we have to keep calling uart_fifo_read from this callback, even if we don't have anywhere to put it... And we're an an ISR so we can't just yield. I think the best we can actually do in this scenario is skip the bytes LOG_ERR
that we're doing so, and bail.

Thoughts?

[petejohanson]

Yup, here's a pretty authoritative Zephyr use of the async API for the shell work. It basically drops the data if there's no room in the ring buffer:

https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/shell/backends/shell_uart.c#L84

(modified to remove some conditional code for clarity)

	do {
		len = ring_buf_put_claim(&sh_uart->rx_ringbuf, &data,
					 sh_uart->rx_ringbuf.size);

		if (len > 0) {
			rd_len = uart_fifo_read(dev, data, len);

			/* If there is any new data to be either taken into
			 * ring buffer or consumed by the SMP, signal the
			 * shell_thread.
			 */
			if (rd_len > 0) {
				new_data = true;
			}

			int err = ring_buf_put_finish(&sh_uart->rx_ringbuf, rd_len);
			(void)err;
			__ASSERT_NO_MSG(err == 0);
		} else {
			uint8_t dummy;

			/* No space in the ring buffer - consume byte. */
			LOG_WRN("RX ring buffer full.");

			rd_len = uart_fifo_read(dev, &dummy, 1);
		}
	} while (rd_len && (rd_len == len));

[petejohanson]

Updated to match.

[Nick-Munnich]

Just as a small note, could you either finish (there's a TODO) & add the config and behavior doc pages to the sidebar or remove them from this PR? The RPC page looks good.

[petejohanson]

Just as a small note, could you either finish (there's a TODO) & add the config and behavior doc pages to the sidebar or remove them from this PR? The RPC page looks good.

I will move the config page to #2268 since that PR makes studio actually end user usable. The RPC docs I will add to the sidebar in this PR since it's part of this core work. That work @Nick-Munnich ?

[Nick-Munnich]

I will move the config page to #2268 since that PR makes studio actually end user usable. The RPC docs I will add to the sidebar in this PR since it's part of this core work. That work @Nick-Munnich ?

Moving them is a good solution, but be aware that you've already added the RPC docs, the other part which was added but not listed in the sidebar is "Studio Unlock" behaviour. I'd guess that should be moved too?

[petejohanson]

I will move the config page to #2268 since that PR makes studio actually end user usable. The RPC docs I will add to the sidebar in this PR since it's part of this core work. That work @Nick-Munnich ?

Moving them is a good solution, but be aware that you've already added the RPC docs, the other part which was added but not listed in the sidebar is "Studio Unlock" behaviour. I'd guess that should be moved too?

Yeah, I saw RPC was already in the sidebar. I've left that for now, but moved the config doc to #2268 and added in the sidebar there.

I wanted to be sure I had the unlock behavior documented, but don't really want to "expose it" until it's actually usable for something. I'd like to keep the doc, but add to sidebar in #2268. Any concerns?

[Nick-Munnich]

I wanted to be sure I had the unlock behavior documented, but don't really want to "expose it" until it's actually usable for something. I'd like to keep the doc, but add to sidebar in #2268. Any concerns?

Best practice would probably be to add it to sidebar and then comment it out in this PR, but I think that's not necessary unless you wanted to. All good from me.

[petejohanson]

I wanted to be sure I had the unlock behavior documented, but don't really want to "expose it" until it's actually usable for something. I'd like to keep the doc, but add to sidebar in #2268. Any concerns?

Best practice would probably be to add it to sidebar and then comment it out in this PR, but I think that's not necessary unless you wanted to. All good from me.

Given the type check from Docusaurus is now angry about unlock docs referencing the config docs that don't exist, I'm just going to move that doc to #2268 also for cohesion.