Move `SensorId` validation to clients, and reply fault when invalid #1683

hawkw · 2024-03-26T19:20:53Z

As described in #1680, the sensors API currently validates SensorIDs in the server task and returns an error when they are invalid. We can simplify the IPC interface by doing this validation on the client side, and reply-faulting any clients who send an invalid ID, instead. This branch does that.

This PR consists of a sequence of commits which are intended to be reviewed individually.

The `SensorId` type represents an index into an array of sensors in the `sensors` server task. Currently, this is validated by the sensors server, returning `SensorError::InvalidSensor` if it is out of range. However, the sensor IDs used by firmware code are hard-coded by code generation and should never be out of range, so in most cases, an invalid `SensorId` should not occur and should be considered a bug. Only `host-spc-comms` will ever request sensor IDs that are out of range, if the upstack code requests an ID that doesn't exist on this device. Therefore, we ought to change the sensor API to use reply-fault on receipt of an invalid `SensorId`, rather than returning an error. This commit prepares for such a change by adding client-side validation of `SensorId`s with a `TryFrom` impl, and a panicking `SensorId::new` This way, the tasks that *can* handle sensor IDs from the outside world (`host-sp-comms`) can validate that sensor IDs received from the outside world are valid _before_ calling into the sensor task API, and reject any invalid IDs. In a subsequent PR, `task_sensor_api` will be updated to remove the `InvalidSensor` error type. A nice side benefit of using the panicking `const fn new` in generated code is that...it's const fn, and the code generation mostly uses it in `const` initializers. So, if any of the code generation accidentally emits an invalid sensor ID, we'll actually get a build-time rather than runtime error, which seems nice!

Currently, a number of IPCs in the `Sensor` API return a `SensorApiError` type, which represents either "invalid sensor ID" or "no reading". Since passing an invalid `SensorId` to the sensors API will now result in a reply-fault, there's no reason to return an error type that can represent that case. This commit changes the `Sensor` API to return `Option<T>`s rather than `Result<T, SensorApiError>`, with `None` representing the "no reading" case. Fixes #1680

Now that we can guarantee all `SensorId`s are valid indices, we can also eliminate all bounds-checked array indexing, and return `RequestError::Fail(ClientError::BadMessageContents)` instead. This shaves off 240B of flash, which is...less than I had hoped.

This shaves off a few more bytes of flash, although we'd also need to get rid of panics in idol-generated code to completely eliminate panic formatting from the sensor task...

The `SensorId` type represents an index into an array of sensors in the `sensors` server task. Currently, this is validated by the sensors server, returning `SensorError::InvalidSensor` if it is out of range. However, the sensor IDs used by firmware code are hard-coded by code generation and should never be out of range, so in most cases, an invalid `SensorId` should not occur and should be considered a bug. Only `host-spc-comms` will ever request sensor IDs that are out of range, if the upstack code requests an ID that doesn't exist on this device. Therefore, we ought to change the sensor API to use reply-fault on receipt of an invalid `SensorId`, rather than returning an error. This commit prepares for such a change by adding client-side validation of `SensorId`s with a `TryFrom` impl, and a panicking `SensorId::new` This way, the tasks that *can* handle sensor IDs from the outside world (`host-sp-comms`) can validate that sensor IDs received from the outside world are valid _before_ calling into the sensor task API, and reject any invalid IDs. In a subsequent PR, `task_sensor_api` will be updated to remove the `InvalidSensor` error type. A nice side benefit of using the panicking `const fn new` in generated code is that...it's const fn, and the code generation mostly uses it in `const` initializers. So, if any of the code generation accidentally emits an invalid sensor ID, we'll actually get a build-time rather than runtime error, which seems nice!

Currently, a number of IPCs in the `Sensor` API return a `SensorApiError` type, which represents either "invalid sensor ID" or "no reading". Since passing an invalid `SensorId` to the sensors API will now result in a reply-fault, there's no reason to return an error type that can represent that case. This commit changes the `Sensor` API to return `Option<T>`s rather than `Result<T, SensorApiError>`, with `None` representing the "no reading" case. Fixes #1680

Now that we can guarantee all `SensorId`s are valid indices, we can also eliminate all bounds-checked array indexing, and return `RequestError::Fail(ClientError::BadMessageContents)` instead. This shaves off 240B of flash, which is...less than I had hoped.

This shaves off a few more bytes of flash, although we'd also need to get rid of panics in idol-generated code to completely eliminate panic formatting from the sensor task...

task/sensor-api/src/lib.rs

task/host-sp-comms/src/bsp/gimlet_bcde.rs

hawkw · 2024-03-26T23:07:17Z

ugh, clippy's mad about some more stuff, gimme a sec...

lib/host-sp-messages/src/lib.rs

task/sensor-api/src/lib.rs

mkeeter · 2024-03-27T13:24:49Z

task/sensor/src/main.rs


        //
        // We pack per-`NoData` counters into a u32.
        //
        let (nbits, shift) = nodata.counter_encoding::<u32>();
-        let mask = (1 << nbits) - 1;
+        let mask = (1u32 << nbits).saturating_sub(1);


Is this to remove overflow checks?

Yeah, I was hoping we could completely get rid of panics --- unfortunately, we can't, as the generated idol server code has some potential panic paths, but eliminating overflow checks here does shave a few bytes off the flash size regardless.

mkeeter

A few more nits, but generally looks good – it's great to see more APIs become infallible!

The `SensorId` type represents an index into an array of sensors in the `sensors` server task. Currently, this is validated by the sensors server, returning `SensorError::InvalidSensor` if it is out of range. However, the sensor IDs used by firmware code are hard-coded by code generation and should never be out of range, so in most cases, an invalid `SensorId` should not occur and should be considered a bug. Only `host-spc-comms` will ever request sensor IDs that are out of range, if the upstack code requests an ID that doesn't exist on this device. Therefore, we ought to change the sensor API to use reply-fault on receipt of an invalid `SensorId`, rather than returning an error. This commit prepares for such a change by adding client-side validation of `SensorId`s with a `TryFrom` impl, and a panicking `SensorId::new` This way, the tasks that *can* handle sensor IDs from the outside world (`host-sp-comms`) can validate that sensor IDs received from the outside world are valid _before_ calling into the sensor task API, and reject any invalid IDs. In a subsequent PR, `task_sensor_api` will be updated to remove the `InvalidSensor` error type. The `host-sp-messages` crate is changed to replace the use of `SensorId` with `u32`, as it deals with _un-validated_ sensor indices received from the host over IPCC. `host-sp-comms` will validate these sensor IDs before calling into sensor IPC methods. A nice side benefit of using the panicking `const fn new` in generated code is that...it's const fn, and the code generation mostly uses it in `const` initializers. So, if any of the code generation accidentally emits an invalid sensor ID, we'll actually get a build-time rather than runtime error, which seems nice!

Currently, a number of IPCs in the `Sensor` API return a `SensorApiError` type, which represents either "invalid sensor ID" or "no reading". Since passing an invalid `SensorId` to the sensors API will now result in a reply-fault, there's no reason to return an error type that can represent that case. This commit changes the `Sensor` API to return `Option<T>`s rather than `Result<T, SensorApiError>`, with `None` representing the "no reading" case. Fixes #1680

Now that we can guarantee all `SensorId`s are valid indices, we can also eliminate all bounds-checked array indexing, and return `RequestError::Fail(ClientError::BadMessageContents)` instead. This shaves off 240B of flash, which is...less than I had hoped.

This shaves off a few more bytes of flash, although we'd also need to get rid of panics in idol-generated code to completely eliminate panic formatting from the sensor task...

Now that `host-sp-messages` no longer depends on `task-sensor-types`, there's really no reason for it to be a separate crate. Thus, let's just move the error types into `task-sensor-api`.