Skip to content
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

Allow SP to set active slot #1808

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Allow SP to set active slot #1808

merged 1 commit into from
Jun 6, 2024

Conversation

labbott
Copy link
Collaborator

@labbott labbott commented Jun 5, 2024

It's useful for the SP to be able to change the active slot. Pull this through. The final finish step no longer implicitly swaps the bank.

It's useful for the SP to be able to change the active slot.
Pull this through. The final finish step no longer implicitly
swaps the bank.
&mut self,
_: &RecvMessage,
) -> Result<SlotId, RequestError<Infallible>> {
Ok(self.pending)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this state retrievable from a HW register? Just thinking of the corner case where update_server restarts and having two sources of truth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok... I see the fetch of HW state below. Looks ok.

@@ -69,5 +69,24 @@ Interface(
),
idempotent: true,
),
"get_pending_boot_slot": (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idl/lpc55-update-server.idol switch_default_image API doesn't fit well enough to reuse here.
Someday we could unify the boot policy APIs.

@labbott
Copy link
Collaborator Author

labbott commented Jun 5, 2024

Fixes #1755

Copy link
Collaborator

@cbiffle cbiffle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a lot of actual driver changes required, looks like!

Inactive = 1,
}

impl TryFrom<u16> for SlotId {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're using this for something other than IPC, carry on.

For IPC specifically, your use of encoding: Hubpack relieves you from needing these instances (and explicit = X discriminator values). FYI.

@@ -519,9 +543,24 @@ impl NotificationHandler for ServerImpl<'_> {
fn main() -> ! {
let flash = unsafe { &*device::FLASH::ptr() };

// If the server restarts we need to fix our pending state
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks like correct handling of task restart.

SerializedSize,
)]
pub enum SlotId {
Active = 0,
Copy link
Contributor

@aapoalas aapoalas Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: From the peanut gallery: Does the naming of the slot IDs come from an outside source, or is it otherwise well understood within Oxide?

From an outsider perspective, it seems weird that apparently there are two banks that you can boot to, so either can be the "actively booted bank" but one is still called "inactive" and the other is "active", regardless of which one is actually currently booted into.

I guess maybe these aren't actually slot/bank IDs but rather just a value that can be set as 1 to say "after restart, switch from current bank to other bank"? If that is the case, would this rather be a "pending boot slot value"?

@labbott labbott merged commit 2f3f789 into master Jun 6, 2024
104 checks passed
@labbott labbott deleted the separate_bank_swap branch June 6, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants