Skip to content

Refactor to use source & headers structure#2302

Open
jakubberkop wants to merge 45 commits intocommaai:masterfrom
jakubberkop:source_and_headers
Open

Refactor to use source & headers structure#2302
jakubberkop wants to merge 45 commits intocommaai:masterfrom
jakubberkop:source_and_headers

Conversation

@jakubberkop
Copy link

@jakubberkop jakubberkop commented Nov 28, 2025

Work in progress for #2171.
Is this the refactor you have in mind? @robbederks

Building of jungle and body binaries are commented out of now, just to get a poc of the refactor.

@robbederks
Copy link
Contributor

Haven't reviewed in depth, but I think this is roughly in the same direction I'd start. Obviously still needs a lot of work to get the subprojects building and all tests passing again, after that I can give it a closer look.

@jakubberkop jakubberkop force-pushed the source_and_headers branch 4 times, most recently from 2e74d6c to 63dfd6b Compare December 5, 2025 22:46
@jakubberkop
Copy link
Author

jakubberkop commented Dec 5, 2025

Ready for a review. @robbederks

  • I have have suppressed few things in the tests/misra/test_misra.sh, most notably the opendbc includes, as they were causing issues and modifying them seemed out of scope for this task.
  • I also suppressed unused function warnings, for functions that were only used in board/bootstub.c, which is also excluded from the analysis.

@jakubberkop jakubberkop marked this pull request as ready for review December 5, 2025 23:01
@jakubberkop jakubberkop force-pushed the source_and_headers branch 2 times, most recently from 0a89478 to 45c876b Compare January 6, 2026 16:33
@jakubberkop
Copy link
Author

@robbederks please take a look when you have a chance.

@robbederks
Copy link
Contributor

@jakubberkop: looks like it builds and flashes, but doesn't boot on the panda jungle and thus doesn't try it on the pandas either. do you have any hardware to test this on? does it boot on pandas? If not I can try to debug it too

@jakubberkop
Copy link
Author

@robbederks: Unfortunately no, I don't have any hardware.
I went once again through all of my changes related to panda jungle, and couldn't find anything, so help in debugging it would be greatly appreciated.

@robbederks
Copy link
Contributor

robbederks commented Jan 12, 2026

@jakubberkop the STM hangs in the bootloader right after initializing the interrupts. I've loaded bootloader.elf in ghidra, and all the interrupt handlers map to the same do {} while(true); loop, so this isn't surprising.

I see you've also had to add an unusedFunction exception to handle_interrupt, which it shouldn't need.

Lmk once you debugged it and would like me to test on hw again

@jakubberkop
Copy link
Author

@robbederks thanks for the help!
As suggested the issues was with the Interrupt handlers. I forgot to add the interrupt_handlers.c to the SConscript, and that didn't result in a undefined reference to a symbol errors, because those functions were marked as weakly linked.

Should be ready for tests on hw.

@robbederks
Copy link
Contributor

Great. I'll try again tomorrow.

Any way you can set linker flags to throw errors for mistakes like these? Failing silently doesn't seem like a great option going forward

@robbederks
Copy link
Contributor

In general we also try to avoid adding more misra suppressions and fixing the underlying issues instead. Haven't looked too extensively for other ones that have been added, but would be good to double check that

@robbederks
Copy link
Contributor

@jakubberkop the RSA signature check was failing in the bootstub. Looks like an off-by-one bug was introduced in rsa.c, I've fixed it.

Can you double check that there are no other changes like this introduced? It's a bit scary in a large PR like this to have functional changes like this since they're very hard to review due to the sheer diff size

@robbederks
Copy link
Contributor

fixed a CAN buffer bug where they weren't in the right memory section, that works now.

for some reason the main red LED breathes at like 2.5x speed with this PR though, didn't see anything obvious why that is the case (is the core running at a different frequency?). The uptime counter does look correct at 1Hz so the main PLLs should be fine

@robbederks
Copy link
Contributor

Figured out the fast LED breathing too: looks like this is the first time the delay function is aligned in memory causing it to run drastically faster. Fixed with #2318 which should be mergeable once HITL CI is back up, after that this PR needs to be rebased on that.

@jakubberkop
Copy link
Author

I went again through all of the files, and compared them the master branch. I have found a few small changes that I introduced by accident and fixed it here (commit b80bcd7). This also removed extraneous misra suppressions.
To prevent silent failures of the linking of interrupt handlers, I have commented the .weak definitions in the linker script (and left them as a reference if we ever remove them).

Interesting that we can get such a huge speedup by just alignment of the delay function. I compared the generated code yesterday, and that the only thing I noticed, but didn't know that it could have had such a big effect. I guess you learn something every day.

@jakubberkop
Copy link
Author

Rebased after #2318

@jakubberkop
Copy link
Author

@robbederks I think this PR is ready.
Is there anything else that you would like to have fixed/improved before merging it?

@robbederks
Copy link
Contributor

trigger-jenkins

@robbederks
Copy link
Contributor

@jakubberkop will review again!

@adeebshihadeh
Copy link
Contributor

trigger-jenkins

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.

3 participants