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

Carefully check all scalar2's for argument order #8

Closed
wants to merge 1 commit into from

Conversation

bunnie
Copy link
Member

@bunnie bunnie commented Dec 9, 2023

Two scalar2's were found to have argument orders flipped in the std re-implementation.

I have included the reference code from the Xous side, can you please check the code comments and see if you agree that the calls to SeekKeyStd and GetTimeUtcMs are flipped?

The other calls I just added lots and lots of comments around them.

Two scalar2's were found to have argument orders flipped in the
std re-implementation.
@bunnie bunnie requested a review from xobs December 9, 2023 15:53
@xobs
Copy link
Member

xobs commented Dec 10, 2023

It does look like GetTimeUtcMs has the opposite endianness to ElapsedMs, which is curious. That's a good find.

Interestingly, the SeekKeyStd calls appear to have been broken before the current patch:

SeekFrom::Start(off) => {
(0, (off as usize & 0xffff_ffff), ((off >> 32) as usize) & 0xffff_ffff)
}
SeekFrom::Current(off) => {
(1, (off as usize & 0xffff_ffff), ((off >> 32) as usize) & 0xffff_ffff)
}
SeekFrom::End(off) => {
(2, (off as usize & 0xffff_ffff), ((off >> 32) as usize) & 0xffff_ffff)
}

GetUtcTimeMs is broken in upstream Rust as well, and I'll have to submit a PR.

Would it be possible to get this as a series of patches instead of all in one patch? I'd like:

  1. One patch that fixes GetUtcTimeMs so that I can submit it upstream in isolation
  2. One patch to fix SeekKeyStd that I can squash into the existing pddb implementation
  3. Remove the surrounding comments. They're handy for this sort of analysis, but their use diminishes once things are stable, and I don't know that I could get the changes merged upstream

If you prefer, I can take the changes and apply them myself.

@bunnie
Copy link
Member Author

bunnie commented Dec 10, 2023

Hmm. Would another way to fix this possibly be to patch Xous to adopt the endianness used in the library? There's no way we're going back to the older libstd code bases, right?

@xobs
Copy link
Member

xobs commented Dec 10, 2023

Correct, we won't be going back to older libstd codebases.

It may make sense to at least patch Core so that SeekKeyStd is swapped around, since libstd has always used the "wrong" order.

As for GetUtcTimeMs, that's up to you. It's perfectly valid to swap it in Core, given that it's a monorepo!

@bunnie
Copy link
Member Author

bunnie commented Dec 10, 2023

I'll close this PR, I've fixed the issues (I think) inside Xous itself.

@bunnie bunnie closed this Dec 10, 2023
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.

2 participants