Fix system attribute addressing#132
Conversation
| // TODO: This should not expect, we should return none in case the chip doesn't have a bootloader | ||
| match n { | ||
| Ok(c) => key.push(c), | ||
| Err(_) => return None, | ||
| } |
There was a problem hiding this comment.
If we are changing this, can we change it directly to the std implementation?
There was a problem hiding this comment.
utf8_decode should be eradicated from this codebase
| } else { | ||
| Ok(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
Add a space after
| /// Check if the bootloader is present on the version of Tock | ||
| pub(crate) async fn bootloader_is_present( | ||
| conn: &mut dyn IO, | ||
| flash_address: u64, |
There was a problem hiding this comment.
| flash_address: u64, | |
| flash_start_address: u64, |
| /// - Err(TockloaderError::SerialReadError): if reading fails on Serial | ||
| pub(crate) async fn read_system_attributes( | ||
| conn: &mut dyn IO, | ||
| flash_address: u64, |
There was a problem hiding this comment.
| flash_address: u64, | |
| flash_start_address: u64, |
| pub(crate) async fn read_system_attributes( | ||
| conn: &mut dyn IO, | ||
| flash_address: u64, | ||
| start_address: u64, |
There was a problem hiding this comment.
| start_address: u64, | |
| app_start_address: u64, |
| // System attributes start at 0x600 and up to 0x9FF. See: | ||
| // https://book.tockos.org/doc/memory_layout#flash-1 | ||
| let address = 0x600; | ||
| let address = flash_address + 0x600; |
There was a problem hiding this comment.
Continue this comment. It start at an offset of 0x600, an example being visible at . Furthermore ,this is also evidenced by the linker script in tock upstream
| result.boothash = Some(decoded_attributes.value.to_string()); | ||
| let has_bootloader = Self::bootloader_is_present(conn, flash_address) | ||
| .await | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
bubble up the error, don't unwrap_or
| } else { | ||
| for current_slot in 0..data.len() { |
There was a problem hiding this comment.
| } else { | |
| for current_slot in 0..data.len() { | |
| } else { | |
| // TODO(eva-cosma): Clean this up/refactor | |
| for current_slot in 0..data.len() { |
You can also do the cleanup in this PR, but you will need to test the new implementation. We can discuss what I don't like, if you want to cleanup
| let sentinel = std::str::from_utf8(&kernel_attr_binary[96..100]) | ||
| .unwrap_or("unknown") | ||
| .to_string(); |
There was a problem hiding this comment.
If we don't have a sentinel, or we can't convert it, throw an error. A very specific one as well, INVALID_SENTINEL. something like that. Sentinels exist to be unchanged and always correct.
| pub flash_address: u64, | ||
| pub start_address: u64, |
There was a problem hiding this comment.
Cool, but I've changed my mind, let's not have 1-to-1 parity with tockloader-python in the naming scheme here cause this is confusing. We can do
/// Previously named 'flash_address' in the python version of tockloader
pub flash_start_address: u64,
/// Previously named `start_address` in the python version of tockloader
pub app_start_Address: u64,| fn default() -> Self { | ||
| Self { | ||
| arch: None, | ||
| flash_address: 0x00000, // this would be actually like -0x10000 |
There was a problem hiding this comment.
How can flash be negative?
| let installed_apps = self.read_installed_apps().await?; | ||
| let system_atributes = self.read_system_attributes().await?; |
5038ab5 to
685a08f
Compare
685a08f to
022dafa
Compare
2 participants
Pull Request Overview
This pull request:
-Adds the Nucleo U545RE-Q Board.
-Adds start address field for known boards.
-Fixes info/erase-apps commands on boards that don't have a bootloader by removing the reading of system attributes(which do not have exist if you don't have a bootloader).
-Change bytes_to_string function to standard library function.
Checks
Using Rust tooling
cargo fmtcargo clippycargo testcargo buildFeatures tested:
erase,info,list,listen