I2C codegen dedupe

[jamesmunns]

Closes #2552

This is the most straightforward de-duplication approach, essentially "pushing down" the knowledge of whether a device requires a rail or not.

This change matches the current behavior, and that of the system prior to #2538, and replaces the pre-existing by-rail-name accessor with the newer by-rail-name accessor (giving it the pre-existing accessor's nicer name).

It feels like this change might not go far enough yet: for all of the devices we are creating, it is probably statically knowable whether that device SHOULD or SHOULD NOT have a rail index. For example, Bmr491 and Tps546B24A do not, and previously had a "dead" rail parameter that I've removed.

For correctness' sake, it feels like we should ideally fail out at compile time if a device SHOULD have multiple rails, and doesn't (according to the app toml), OR if a device SHOULDN'T have multiple rails, but does (according to the app toml).

I'm going to take a look to see if there's a way to go further with this, but opening this to share for now.

[jamesmunns]

Gunna take another crack at this to see if I can distinguish the two codegen groups (with or without rails) more distinctly.

[jamesmunns]

I made some progress, but actually I don't think a better fix is possible without some significant work to task/power. The BSPs create an array of PowerControllerConfig, which needs a constructor to create a device (from the generated helper function), and there are some endpoints like raw_pmbus_read_* that:

• Work generically (sort of?) over the different devices
• Don't actually listen to whether the device SHOULD or SHOULD NOT use rail commands

I'm probably going to punt on this, but it might be worth taking a look at how task-power works today if we want to lean harder into "use info from codegen to statically determine if we should be using rail-based PAGE commands or not".

CC @hawkw.

[evan-oxide]

I'm trying to understand some context: What's the purpose of calling the index None instead of 0? Is there any place where we need to distinguish between the two, and where we don't just turn the None back into 0 with .unwrap_or(0)?

I'm trying to think of a situation that complicates the meaning of None vs 0... What if we had a device with multiple rails but we only needed to use one, so we only listed the first rail in the manifest? That seems like a situation where the index of the single rail should be 0, not None. It might be an unrealistic situation, but would it break anything? Actually I guess you want that to be a compile error, according to the PR description.

[jamesmunns]

I think this is a good question! Right now, there are some places where we use the existence of None vs Some(0) to decide whether we send a PAGE command. There are some devices in practice where sending a PAGE command isn't allowed. "Get PMBUS Status" handles this correctly, but some other places (like task-power), particularly for raw debugging commands, doesn't.

I could make this a separate metadata piece, for example returning (I2cDevice, u8, bool) (or making a struct like { dev: I2cDevice, port: u8, is_paged: bool }, which might be less implicit than using Option<u8> to represent the port.

What if we had a device with multiple rails but we only needed to use one, so we only listed the first rail in the manifest?

In this case, it would have to be the 0th page we used, and it would have to be okay that we never send paging commands.

Since paging is a stateful swap, this should be fine, IMO.