Skip to content

Leave circular dependency hell (adopt source/header model)#2175

Closed
aubsw wants to merge 22 commits intocommaai:masterfrom
aubsw:leave-dependency-hell
Closed

Leave circular dependency hell (adopt source/header model)#2175
aubsw wants to merge 22 commits intocommaai:masterfrom
aubsw:leave-dependency-hell

Conversation

@aubsw
Copy link
Copy Markdown
Contributor

@aubsw aubsw commented Mar 18, 2025

This is to address #2171

Unfortunately, for this to work in the end, I think we need to do the same to opendbc, since that library being header-only breaks our build.

For local testing, I patched opendbc, and it was a fairly small patch.

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Mar 18, 2025

@robbederks I think for this work, we will need source-and-header-ify https://github.com/commaai/opendbc/tree/master/opendbc/safety, since we would need to #include those headers here.

I got build working locally by patching safety.h & co.

I'm also happy to try to break this work up in to several smaller PRs, tho that may take some time because it's difficult to break out individual headers without generating conflicts at the linking stage.

@robbederks
Copy link
Copy Markdown
Contributor

Thanks. For now, the bounty is locked to #2172, if that attempt stalls it'll be unlocked again.

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Mar 19, 2025

Aw crud my bad. I started working on this before it was locked–didn't realize what was going on 😞 .

@robbederks
Copy link
Copy Markdown
Contributor

@aubsw the bounty is unlocked if you want to continue!

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 2, 2025

I'm in!

I will draft a more detailed plan for breaking the change into smaller PRs today.

I did the hard part already which is getting the code to compile. Now just need to make the diff a bit more digestible + supporting PR in the opendbc repo.

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 2, 2025

@robbederks I'm prototyping my merge plan and am a bit behind. I will follow-up on the merge plan with a clear head tomorrow morning.

@aubsw aubsw force-pushed the leave-dependency-hell branch from 49cd5d7 to a8ac929 Compare April 7, 2025 23:56
f"{panda_root}/../opendbc/safety/",
f"{panda_root}/../opendbc",
f"{panda_root}/../opendbc/safety",
f"{panda_root}/include/",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it possible to use something like f"{panda_root}/include/**" to avoid having to list every folder?

Copy link
Copy Markdown
Contributor Author

@aubsw aubsw Apr 8, 2025

Choose a reason for hiding this comment

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

Suprisingly, niether GCC nor SCons provides recursive directory include search. We have a few options:

  1. leave it how it is
  2. write a helper function to recursively get search paths
  3. add only top-level directories to the search path, adjust all header includes to use the full path. I.e. #include "registers.h" would become #include "drivers/registers.h" everywhere.

I kind of prefer option 3 because it clarifies the directory structure of included headers in the source code.

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.

[$500 Bounty] Refactor panda to use source + header file structure

2 participants