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

chore: Cleanup remaining userlib namespace imports #1857

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aapoalas
Copy link
Contributor

@aapoalas aapoalas commented Aug 23, 2024

This PR ends my work of walking through the hubris code base, removing userlib namespace imports.

This time I did a search-and-replace on all of these to clear them out. I currently see some build issue about notifications with app/lpc55xpresso/app-sprot.toml; how the notifications should get built is not quite clear to me.

Another interesting file is task/template/src/main.rs where we have this comment:

// NOTE: you will probably want to remove this when you write your actual code;
// we need to import userlib to get this to compile, but it throws a warning
// because we're not actually using it yet!
#[allow(unused_imports)]
use userlib::{};

I'm not sure if I should add some imports here or not.

@@ -923,7 +923,7 @@ impl I2cController<'_> {

#[rustfmt::skip]
i2c.oar1.modify(|_, w| { w
.oa1en().clear_bit() // own-address disable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: Zed editor did these format fixes automatically, so they're kind of "slipping in" on the side.

@aapoalas aapoalas changed the title chore: Remove the rest of userlib namespace imports chore: Cleanup remaining userlib namespace imports Aug 24, 2024
@aapoalas
Copy link
Contributor Author

aapoalas commented Oct 2, 2024

@cbiffle My apologies for the ping: Would you be amenable to taking a look at this? All CI tests have been fully green, only app/lpc55xpresso/app-sprot.toml has some sort of local build failure about missing notifications that seemed like it might be a configuration failure in that board's part. It is not being tested by CI so I am not sure if that is even relevant.

@aapoalas aapoalas force-pushed the chore/userlib-namespace-imports branch from 633adda to c453676 Compare October 11, 2024 02:07
@aapoalas
Copy link
Contributor Author

The remaining lpc55xpresso app-sprot.toml dist xtask passes now as well. I had to bump some stack sizes and copy SP_TO_ROT_JTAG_DETECT_L and ROT_TO_SP_RESET_L pin definitions from oxide-rot-1's app.toml. I have no idea if these are correct, but it seems fairly likely that if the xpresso build is still being used then it is somehow accidentally pulling in the pin definitions from the rot-1 configuration, in which case they should still be correct.

Though oops, I only now actually checked whether app-sprot.toml works on the master branch and it doesn't, so I guess that was wasted work.

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.

1 participant