Skip to content

Delete duplicated code from panda#2186

Closed
aubsw wants to merge 24 commits intocommaai:masterfrom
aubsw:delete-dup-panda-code-1
Closed

Delete duplicated code from panda#2186
aubsw wants to merge 24 commits intocommaai:masterfrom
aubsw:delete-dup-panda-code-1

Conversation

@aubsw
Copy link
Copy Markdown
Contributor

@aubsw aubsw commented Apr 11, 2025

details

  • utils.h and can.h are approximately duplicated from opendbc, which is making [$500 Bounty] Refactor panda to use source + header file structure #2171 hard to land because it results in duplicated symbols
  • since opendbc has not yet adopted source/header style, we can't include many headers from opendbc in panda without getting multiple defintions of the same symbols (compilation errors)

@robbederks
Copy link
Copy Markdown
Contributor

Diff looks good to me, can merge once the tests are fixed. I've bumped the opendbc ref too.

Copy link
Copy Markdown
Contributor Author

@aubsw aubsw left a comment

Choose a reason for hiding this comment

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

@robbederks not sure why, but these changes have exposed some new cppcheck failures.

Mostly just conditionally unused functions (i.e. some functions only get used when ENABLE_SPI is true). Pls advise (see comments).

Comment on lines +75 to +77
PANDA_SOURCES="$PANDA_DIR/board/ --enable=all --file-filter=*.c \
-i $PANDA_DIR/board/safety.c \
-i $PANDA_DIR/board/jungle/main.c \
-i $PANDA_DIR/board/bootstub.c"
Copy link
Copy Markdown
Contributor Author

@aubsw aubsw Apr 14, 2025

Choose a reason for hiding this comment

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

@robbederks should we ignore all non-main.c files?

If we don't ignore, we'll have a lot more cppchecks failures.

The old behavior was to only check board/main.c.

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.

Ideally we check everything I think, but we should especially check safety.c and main.c if that covers all of the panda app code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Kk. Looking into it. It may require some more cahnges to opendbc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I cleared up board/main.c and safety.c.

Currently both bootstub.c and jungle/main.c are in a MISRA-incompliant state. I think we should address those later for the sake of progress.

@aubsw aubsw changed the title Delete utils.c and can.h from panda Delete utils.h and can.h from panda Apr 14, 2025
@aubsw aubsw force-pushed the delete-dup-panda-code-1 branch from b1d9ec6 to 6610a3d Compare April 14, 2025 16:13
@aubsw aubsw marked this pull request as draft April 14, 2025 16:50
@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 14, 2025

Marked it a draft until #2188 and #2189 land.

@aubsw aubsw force-pushed the delete-dup-panda-code-1 branch from b6b1b8d to 718f5db Compare April 15, 2025 14:22
@aubsw aubsw marked this pull request as ready for review April 15, 2025 14:27
@aubsw aubsw requested a review from robbederks April 15, 2025 14:30
@aubsw aubsw force-pushed the delete-dup-panda-code-1 branch from 8892d9e to 658e5c6 Compare April 17, 2025 13:40
@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 17, 2025

Rebased, cleaned up a few odds and ends.

@aubsw aubsw requested a review from robbederks April 17, 2025 14:04
@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 18, 2025

Anything for me to do regarding this failed hitl run? I don't think I have access to https://jenkins.comma.life/job/panda/job/tmp-jenkins-2186/2/display

@robbederks
Copy link
Copy Markdown
Contributor

Jungle v2 doesn't seem to boot with this PR (gets stuck resetting after flashing). Do you have any hardware to test this out on?

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 18, 2025

oh no! 🫠

I only have comma 3x. I would need https://comma.ai/shop/panda-jungle to test out the jungle boot, yeah?

Does the non-jungle panda boot without issues?

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 18, 2025

I found a lead for what the issue might be... Not sure if I can trigger jenkins tho

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented Apr 22, 2025

Sorry for the n00b question, but do you think I could debug this with a blue pill stm board?

@robbederks
Copy link
Copy Markdown
Contributor

Sorry, I'm OOO this week so reply time might be a bit slower

@commaai commaai deleted a comment from aubsw May 12, 2025
@robbederks
Copy link
Copy Markdown
Contributor

trigger-jenkins

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented May 14, 2025

@robbederks is the HITL test still failing?

@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented May 14, 2025

Adeeb was unhappy with this PR in the dev call today, which is understandable as it's not the ideal solution.

I think that the ideal solution would probably be to make opendbc use source/header style so that we can easily link the safety code into panda.

@aubsw aubsw force-pushed the delete-dup-panda-code-1 branch from e1552af to 4b121f5 Compare May 15, 2025 14:36
@aubsw aubsw force-pushed the delete-dup-panda-code-1 branch from 4b121f5 to fa019c5 Compare May 15, 2025 14:39
@aubsw
Copy link
Copy Markdown
Contributor Author

aubsw commented May 15, 2025

OK I took another hard look. Found a waaay simpler way to do this step.

We may still need to make another PR to deal with linking safety code properly for the final PR, but if we can land this one first, that PR will be way less risky.

@aubsw aubsw force-pushed the delete-dup-panda-code-1 branch from 95ce85a to 6e937bb Compare May 15, 2025 21:17
# TODO: this should be a "pip install" or not even in this repo at all
RUN git config --global --add safe.directory $PYTHONPATH/panda
ENV OPENDBC_REF="da0a5e3d2b3984b56ebf5e25d9769f5c77807e4d"
ENV OPENDBC_REF="d631299b8d49253fdeb5b709684af031ab1d18be"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Needs to be updated after commaai/opendbc#2248 lands

@aubsw aubsw changed the title Delete utils.h and can.h from panda Delete duplicated code from panda May 15, 2025
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