-
Notifications
You must be signed in to change notification settings - Fork 11
feat: reshuffle_apps #103
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: reshuffle_apps #103
Conversation
|
I temporarily changed the install function to just call reshuffle_apps and a new TAB to the existing list of apps, find a valid permutation and then write to the board. This works fine for installing C apps. When trying to install a Rust app, there are a few problems that occur:
I tried choosing the correct TBF manually, yet somehow tockloader-rs found that the app should start at
|
|
Is it always 0x80 bytes off? This, intuitively, feels like a header is not being skipped in the calculation, or maybe due to page-alignment? Do we do page-alignment for fixed apps? Until i get back to the office, try looking at elf2tab, they might create a padding app. Also, try setting header_size to 0 or 16 (the length of the base header). Maybe the parsing algorithm can help you: https://github.com/WyliodrinEmbeddedIoT/tockloader-rs/blob/main/tbf-parser%2Fsrc%2Fparse.rs#L129 Edit: I'm pretty sure if you create a base header with header length 16 (bytes), you will have just created a padding app. Note that this does mean padding apps cannot be less than 16 bytes long or "0x10" in size |
|
Installing via TBF selection should be changed, as |
b017563 to
dcd9db0
Compare
eva-cosma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Now that we have the IO rewrite, can we merge getting app attributes form probe-rs and serial?
I still have to review reshuffle and install, otherwise LGTM
|
@addrian-77 Rebase on top of the latest main (don't forget to |
Signed-off-by: addrian-77 <lunguadrian30@gmail.com>
Signed-off-by: addrian-77 <lunguadrian30@gmail.com>
a9798b9 to
d9c0012
Compare
Signed-off-by: Eva Cosma <eva.cosma.mail@gmail.com>
eva-cosma
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so, I rewrote reshuffle apps in a way that makes it so we don't depend at all on where the apps come from or what is the deal with them. It also minimizes adding unnecessary/hyper-specific fields in other structures.
I haven't refactored everything: there are still plenty of errors. I just did the reshuffle_apps function, I'll leave the rest to you.
This pass has revealed some issues though:
- We do not make use of the IO rewrite in the app_attributes. We no longer need to differentiate between probe_rs and serial getting of apps thanks the IO rewrite.
- PAGE_SIZE is board dependant, and should be in board_settings. This does however spread to the
writefunction needing to have a page_size passed to it. the write function itself I would only add as an argument the page_size, and above that I would add board_setting - The algorithm you made is great, but does not tackle the main problem - rust apps being able to be put in multiple places. Adding permutations to that won't be feasible. I suggest we either just implement what tockloader has already, and tackle mixed at a later date, or we go full-force with the constraint satisfaction algorithm.
- I don't think we care to minimize for minimum padding, as long as everything fits. Can be an option in the future.
- I suggest we split this PR in two again, one for the IO rewrite, one for reshuffle. We won't merge them in main but in a separate feature branch, where we stack tab rework + io rewrite + reshuffle and only after we merge. We could have tab be merged directly into main since it only adds features, while io rewrite removes install, so that is why it must go into a feature branch. Opinions?
| /// | ||
| /// See also <https://book.tockos.org/doc/tock_binary_format> | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need clone
| pub fn from_app_attributes(_app_attributes: &AppAttributes) -> Self { | ||
| todo!("Implement TockApp::from_app_attributes") | ||
| } | ||
|
|
||
| pub fn from_tab(_tab: &Tab) -> Self { | ||
| todo!("Implement TockApp::from_tab") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement and use these
| address - address % ALIGNMENT | ||
| } | ||
|
|
||
| pub fn create_pkt(configuration: Vec<AppAttributes>, mut app_binaries: Vec<Vec<u8>>) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't make sense to be in reshuffle, move it to install
| } | ||
|
|
||
| // this function takes a rust app's fixed address and aligns it down to ALIGNMENT (1024 currently) | ||
| fn align_down(address: u64) -> u64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need to also align_down in places in reshuffle itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, align_down is used in reconstruct_app, for rust apps
|
Also please don't squash for now. Let's do it at the end after we are sure everything is okay, since I deleted some of your old code, which you might need back at some point. |
|
In the last commit I mostly implemented all the changes requested. I tried installing a C app for now and it worked. This PR needs some cleanup and maybe the traits have to be moved somewhere else. |
| return TockApp::Fixed(FixedApp { | ||
| idx: None, | ||
| board_address: None, | ||
| candidate_addresses: vec![address as u64], // (adi): change this when tbf selector gets merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No? This is just correct? Since if it is an app attribute, it means that it's already installed
|
|
||
| fn as_intermediate_index(&self, install_address: u64) -> IntermediateIndex { | ||
| IntermediateIndex { | ||
| // useless? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete function(s) then
| // tab: Option<&Tab>, settings: &BoardSettings) -> Option<AppAttributes> | ||
| let arch = settings | ||
| .arch | ||
| .as_ref() | ||
| .ok_or(InternalError::MisconfiguredBoardSettings( | ||
| "architechture".to_owned(), | ||
| )) | ||
| .unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just get the arch then. Unwraps baaad :( and we can return an error above from the calling code
|
|
||
| // extract the binary | ||
| // this should be changed to accomodate candidate_addresses | ||
| let binary = tab.extract_binary(arch).expect("invalid arch"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The amount of unwraps/expects scares me
(message inspired from a great thinker)
| pub fn from_tab(_tab: &Tab) -> Self { | ||
| todo!("Implement TockApp::from_tab") | ||
| /// This function reads the full binary of a given app | ||
| pub async fn read_binary(&mut self, conn: &mut dyn IO) -> Result<Vec<u8>, TockloaderError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does a Tock app need to know how to read a binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of it is that it's a source-erased version of an app, so that reshuffle can just play around with it, and the calling code can then just re-add the source to it when needed
| let mut footer_offset = binary_end_offset; | ||
| let mut footer_number = 0; | ||
|
|
||
| while footer_offset < total_size { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what we are doing with the footers here
| #[derive(Clone)] | ||
| pub struct FlexibleApp { | ||
| idx: Option<usize>, | ||
| board_address: Option<u64>, // None if not installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ideally should not be needed. We do not care for where an application is installed. The calling code can deal with that. TockApp/FlexibleApp/FixedApp are just concepts that do not leave reshuffle_apps. These exist in a vacuum. The calling code is responsible to then understand the new order.
| } | ||
| } | ||
|
|
||
| // a vec of these is returned by reshuffleapps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this commend doing here? If you wanted to make a doc-comment, you missed a /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just notes for myself, my bad :))
tockloader-lib/src/lib.rs
Outdated
| &mut self, | ||
| address: u64, | ||
| pkt: Vec<u8>, | ||
| settings: &BoardSettings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is geniuenly unfortunate that we need to pass board settings to this. We could keep it like this for now, but I'm thinking of maybe attaching the board settings to a connection, like inside the struct. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
1e03e51 to
ee1b2c6
Compare
Signed-off-by: addrian-77 <lunguadrian30@gmail.com>
Signed-off-by: addrian-77 <lunguadrian30@gmail.com>
Signed-off-by: addrian-77 <lunguadrian30@gmail.com>
c019c79 to
26e5438
Compare
Signed-off-by: addrian-77 <lunguadrian30@gmail.com>
f6cd01f to
42fbaf8
Compare
| } | ||
| } | ||
|
|
||
| Some(saved_configuration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a check if we didn't find a permutation that works in the first 100_000 to return None

Pull Request Overview
This pull request adds the reshuffle_apps feature.
TODO or Help Wanted
This pull request still needs an implementation for creating padding apps
Checks
Using Rust tooling
cargo fmtcargo clippycargo testcargo buildFeatures tested:
GitHub Issue
This pull request closes <Implement reorder apps function #102>