Print GPIO pin configuration for humility gpio --input --with-config
#574
Conversation
|
This PR does not build on my Mac (and CI suggests that it also fails on Windows and illumos): I believe that's because you specified the |
|
One big thing that needs to be fixed is making this generic across chips (or printing an appropriate error message). Right now, it would happily try to read the STM32H7's memory locations for any chip that's attached. You could fix this with a trait InputConfig {
type PinConfig: Display;
fn get_pin_config(ctx: &mut ExecutionContext, port: &str, pin: u32) -> Self::PinConfig;
}Then, the |
|
Non-STM32H7 support would be nice to have. I've only done some generalization for the STM32H7 series for now and output error messages for unknown chips. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 104 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 103 | 1 | 0 | 0 |
Click to see the invalid file list
- cmd/gpio/src/stm32h7.rs
lzrd
force-pushed
the
gpio-config-dump
branch
3 times, most recently
from
932a51a to
2d95254
Compare
| results, | ||
| ) | ||
| } else { | ||
| Err(anyhow!( |
There was a problem hiding this comment.
Nit: you could use bail!(..) instead of Err(anyhow!(..))
lzrd
force-pushed
the
gpio-config-dump
branch
from
b78e3f4 to
3ae551f
Compare
$ humility gpio --input -with-config --pins B:0,B:14,E:1 humility: attached to 0483:3754:002F00174741500820383733 via ST-Link V3 B:0 = 0 Analog:OutPushPull:LowSpeed:NoPull:AF0:Unlocked:InZero:OutZero B:14 = 1 Input:OutPushPull:HighSpeed:NoPull:AF5:Unlocked:InOne:OutZero E:1 = 0 Alternate:OutPushPull:VeryHighSpeed:NoPull:AF12:Unlocked:InZero:OutZero ``` If no pins are specified, then the pin names, values and config settings are printeed one per line for all GPIO pins.
Applied several suggestions from Matt.
Co-authored-by: Matt Keeter <[email protected]>
The stm32h7 crate and the stm32-metapac crates are not practical given that they cause difficulties in compiling humility for multiple OS environments and they require a good bit of code to coerce into making generalized GPIO register decoding. Just define the one needed structure.
- Updated documentation to reflect GPIO configuration output specific to chip configuration. - Non-existant pins are not printed. - Added tests for the new behavior.
lzrd
force-pushed
the
gpio-config-dump
branch
from
3ae551f to
c3533ce
Compare
| /// cache is chip-independent and works with any architecture that organizes | ||
| /// GPIO pins into groups/ports with contiguous register blocks. | ||
| pub struct ConfigCache { | ||
| cache: BTreeMap<char, Vec<u8>>, |
There was a problem hiding this comment.
We never store multiple register block types, so this should be made generic across a register block type T. This would let us avoid needing to parse from bytes elsewhere.
There was a problem hiding this comment.
I'm ok with this separation right now. The code that knows about STM32H7 registers doesn't do IO, doesn't hold state, etc. The code that does the IO is just told to that if you want to talk about a pin group, you need to get me this data. There needs to be a from_bytes somewhere to translate the raw data from the device. I think this approach would work well for non STM32H7 type devices as well. But, I'd like to have a motivating case before making a change here.
| ); | ||
| } | ||
|
|
||
| (Some(part.max_group()), Some(part)) |
There was a problem hiding this comment.
I don't understand why you're redundantly storing both part.max_group() and part separately. Below, some code checks for the presence of part (then calls max_group() on it again), while other code uses the max_group variable. You could just store part and call max_group() as needed.
| // Check if pin exists (group exists and pin within range) | ||
| let pin_exists = | ||
| if let (Some(g), Some(part)) = (group, chip_part) { | ||
| g >= 'A' |
There was a problem hiding this comment.
This repeats the logic from group_exists above.
| None => { | ||
| // Skip ports that don't exist on this chip variant | ||
| if !group_exists { | ||
| continue; |
There was a problem hiding this comment.
Should we print something if the user requests a non-existent group?
There was a problem hiding this comment.
"None" is printed for pins that don't exist or a '.' in the simple grid form.
| context: &mut ExecutionContext, | ||
| part: &Stm32H7Part, | ||
| group: char, | ||
| ) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
If you take my suggestion to make ConfigCache generic, this should just return a RegisterBlock (read into it with zerocopy)
There was a problem hiding this comment.
I don't disagree with the suggestion on ConfigCache, but I'll wait for the next chip type to see how that plays out.
|
|
||
| for (ndx, arg) in args.iter().enumerate() { | ||
| let group = | ||
| arg.2.chars().next().ok_or(anyhow!("invalid group '{}'", arg.2))?; |
There was a problem hiding this comment.
Here, we return an error if the user specifies an invalid group; in the non-config routine, we allow it. Resolve this inconsistency one way or the other?
| "Warning: Unknown STM32H7 chip '{}'. Using default GPIO layout (groups A-K).", | ||
| chip | ||
| ); | ||
| eprintln!( |
There was a problem hiding this comment.
I don't think this second warning is needed, because show_gpio_with_config will bail with a similar error.
|
|
||
| // Warn about unknown chips | ||
| if !part.is_known() { | ||
| eprintln!( |
There was a problem hiding this comment.
Use humility::warn! instead?
| } | ||
| } | ||
| ); | ||
| } else if pin >= part.pins_in_group(group) { |
There was a problem hiding this comment.
This is identical to the branch above, merge them?
| |ctx, g| read_gpio_register_block(ctx, part, g), | ||
| )?; | ||
|
|
||
| let max_pin = part.pins_in_group(group); |
There was a problem hiding this comment.
Just iterate from 0..max_pin here?
| max_group: 'I', | ||
| }, | ||
| Stm32H7Part { | ||
| com_part_no: "STM32H743AGI6", |
There was a problem hiding this comment.
Spot-checking this one example, it doesn't look correct. Here is the UFGBA pinout:
Notice that it has PJ0/1 and Port I is partial (e.g. PI4 does not exist). The assumption that pins are tightly packed in groups of 16 is an oversimplification.
More generally, I think that the c3533ce commit is trying to do too much (+1600 lines), and is not worth its weight (e.g. because it's incorrect here).
I'd be fine with the old assumptions of Ports A-K / 176 pins, as long as we check against a small list of blessed chips IDs (instead of trying to support many STM32 part).
2 participants
$ humility gpio --input -with-config --pins B:0,B:14,E:1
humility: attached to 0483:3754:002F00174741500820383733 via ST-Link V3
B:0 = 0 Analog:OutPushPull:LowSpeed:NoPull:AF0:Unlocked:InZero:OutZero
B:14 = 1 Input:OutPushPull:HighSpeed:NoPull:AF5:Unlocked:InOne:OutZero
E:1 = 0 Alternate:OutPushPull:VeryHighSpeed:NoPull:AF12:Unlocked:InZero:OutZero