Fix read efuse #969
Fix read efuse #969
Conversation
bjoernQ
force-pushed
the
fix-read-efuse
branch
from
e30b63b to
b5504d3
Compare
|
The new API seems promising, though the block sizes are still wrong for at least ESP32. Or well, the block size isn't wrong if you expect it to be just the size of the eFuse block, but that's not what we currently actually want out of it. Instead what we actually want out of the block sizes is the size that we have to add to the block 0 address to get to the next block's read address. The problem with ESP32 is that for block 0 there's first the read registers followed by the write registers, which is then followed by the following blocks. Since it's already broken I think it's fine to not fix it right away though. The way I'm thinking about fixing this is by replacing the block size array with an array of block structures which would contain the actual read address for the block instead. I've already started on this but haven't yet finished it. |
|
Yes ESP32 is different here and the whole way how the blocksize is "estimated" is not great (as per the comment I added to the code) I think having that changed in a follow up PR is the best way to go |
kyrias
left a comment
kyrias
left a comment
There was a problem hiding this comment.
I find the implementation kind of hard to follow (it would probably be easier to understand what was going on if the loop adjusted the bit offset such that it would loop over full words and therefore only need to perform the read in one place at the top of the loop, rather than potentially reading words is three different places), but from my tests it seems to behave as expected, both for fields wider than 32 bits and for fields spanning multiple words.
| fn block_address(chip: &Chip, block: u32) -> u32 { | ||
| let block0_addr = chip.efuse_reg() + chip.block0_offset(); | ||
|
|
||
| let mut block_offset = 0; | ||
| for b in 0..block { | ||
| block_offset += chip.block_size(b as usize); | ||
| } | ||
|
|
||
| block0_addr + block_offset | ||
| } |
There was a problem hiding this comment.
Since this is the same logic as read_efuse_raw, any reason for not making this a method so the logic is shared between the two methods?
| fn read_raw(connection: &mut Connection, addr: u32) -> Result<u32, Error> { | ||
| connection.read_reg(addr) | ||
| } |
| // Represent output value as a bytes slice: | ||
| let mut output = std::mem::MaybeUninit::<T>::uninit(); | ||
| let mut bytes = unsafe { | ||
| std::slice::from_raw_parts_mut(output.as_mut_ptr() as *mut u8, std::mem::size_of::<T>()) | ||
| }; |
There was a problem hiding this comment.
Could we get a comment explaining that this is safe due to the bytemuck::AnyBitPattern bound?
Closes #960
Closes #968
This is an overhaul of our efuse reading API.
The original function is now deprecated in favor of
read_efuse_le(which is mostly just a port of the code found in esp-hal).Additionally there was a problem in the xtask in calculating the block size - in general the whole approach is something we might want to reconsider.
On a side note: The current API is quite easy to use in a wrong way. e.g.
Chip::Esp32c2.read_efuse_le::<[u8;16]>(flasher.connection(), efuse::esp32c3::OPTIONAL_UNIQUE_ID)is valid code which will produce garbage.(Guess how I learnt about this!)
This also fixes some problems which are even visible when using the espflash CLI (look at the shown revision and MAC)