API change in saul/phydat

[Teufelchen1]

Hi! 🐆

This:

rust-riot-wrappers/src/saul.rs

Lines 488 to 502 in 409a5be

	/// String representation of a given unit (e.g. `V` or `m`)
	#[doc(alias = "phydat_unit_to_str")]
	pub fn name(self) -> Option<&'static str> {
	unsafe { riot_sys::phydat_unit_to_str(Self::to_c(Some(self))).to_lifetimed_cstr()? }
	.to_str()
	.ok()
	}
	/// Like [`.name()`](Unit::name), but with additional names like "none" or "time".
	#[doc(alias = "phydat_unit_to_str_verbose")]
	pub fn name_verbose(self) -> Option<&'static str> {
	unsafe { riot_sys::phydat_unit_to_str_verbose(Self::to_c(Some(self))).to_lifetimed_cstr()? }
	.to_str()
	.ok()
	}

needs to be changed to something like this:

#[doc(alias = "phydat_unit_write")]
pub fn name(self) -> Option<&'static str> {
    unsafe {
        let mut buffer: char = [0; 10];
        riot_sys::phydat_unit_write(buffer, 10, Self::to_c(Some(self)));
        str::from_utf8(buffer)
    }.ok()
}

in order for this PR RIOT-OS/RIOT#20529 to get merged. Otherwise RIOT/examples/rust-gcoap can not be build.

How do we approach this? 🐔 🔁 🥚

[chrysn]

Generally, just in a PR here, we merge, update the wrappers, merge the PR.

In this case, it's trickier: the wrappers API promises to return a long-lived string reference (essentially a pointer and a length), which judging from your code the C API can't provide any more. Is the result really composed these days (say, from different places in memory)?

[Teufelchen1]

I am afraid it is:
phydat.h

/**
 * @brief   Print a unit
 *
 * @param[in] unit          unit to print
 */
void phydat_unit_print(uint8_t unit);

/**
 * @brief   Write the string representation of the given unit into the given
 *          buffer
 *
 * @param[out]  dest        destination buffer to write to
 * @param[in]   max_size    size of the buffer at @p dest
 * @param[in]   unit        unit to convert
 *
 * @return  Number of bytes written
 * @retval  -EOVERFLOW      buffer at @p dest is too small
 * @retval  -EINVAL         invalid unit in @p unit
 *
 * @warning The function will never write a terminating zero byte
 * @note    If you pass `NULL` for @p dest, it will return the number of bytes
 *          it would write (regardless of @p max_size)
 */
ssize_t phydat_unit_write(char *dest, size_t max_size, uint8_t unit);

[chrysn]

Ah, this is one of those Harvard architecture workarounds. Worst case the lookup can be turned to always return None; are there viable alternatives?

[chrysn]

The ideal answer I'd like to give is "here's what we need for Rust to just do the right thing on AVR and the rest of the world", but without a running AVR port I can't give that yet :-(

[maribu]

This may not be popular, but I think the honest thing to do is to acknowledge that large read-only databases in address space is something we cannot provide in a portable embedded OS. Hence, the Rust API should also be adapted.

Also: For users the new C API is actually more convenient. The two main use cases is printing out the information or writing it into a buffer as part of constructing some CBOR / JSON / foobar-serialization. A similar API change on the Rust side will also be more convenient for new users and quick to fix for existing users.

[chrysn]

On Wed, Apr 03, 2024 at 08:37:13AM -0700, Marian Buschsieweke wrote: read-only databases in address space is something we cannot provide

If we do decide to acknowledge that, I can go through the riot-wrappers, and produce another bunch of cases that will need to be removed as well. Off my head, that's thread names, the board name, the device names of registered SAUL sensors, and CoAP resource paths.

A similar API change on the Rust side will also be more convenient for new users and quick to fix for existing users.

It's not, because writing in Rust has control with the receiver (like, the receiver might be writing into a rope rather than a string), so passing a buffer into which to write doesn't work well. At any rate, that's probably beside the point -- what matters is: Can we have APIs that return string (or other data) pointers or not.

[maribu]

that's thread names

that doesn't apply, as this is not read-only. With DEVELHELP=1 thread names are stored in the thread-control-block inside the thread's stack and can be overwritten with a different thread name, if the previous owner of the TCB and stack finished and a new thread is launched reusing the stack.

the board name

that's not a large read-only database

the device names of registered SAUL sensors

will also not be that large, only a tiny subset of all possible device names will actually be baked in.

Can we have APIs that return string (or other data) pointers or not.

For large read-only databases not. If we could limited the number of entries to those only actually relevant in a given firmware, it wouldn't be much of an issue anymore.

Maybe part of the disagreement is because on the surface this looks like special treatment for exotic Harvard MCUs. But many real-world boards have very limited flash that is mapped into the data address space, but do have flash that is not mapped. E.g. think of a von-Neumann MCU with a micro SD card reader; or the PineTime with SPI attached flash. There is a lot of value in being able to move large stuff out of the data address space.

[chrysn]

I'd be more convinced if I such external storage were actually used here, but am convinced enough to not block this. I'll follow up with a PR on the riot-wrappers side, and ask you for feedback there.