Skip to content

Conversation

@da-gazzi
Copy link

@da-gazzi da-gazzi commented Apr 9, 2025

These changes make the pulp-nnx library more portable:

  • eliminate the dependency on pmsis and the siracusa BSP by making them optional
  • #define's of some architectural parameters are now wrapped in #ifndef so they can be overwritten at compile time

@FrancescoConti FrancescoConti requested a review from lukamac April 10, 2025 12:49
@FrancescoConti
Copy link
Member

Thanks @da-gazzi ! I asked @lukamac for a review

@lukamac
Copy link
Collaborator

lukamac commented Apr 10, 2025

@da-gazzi you are the first fork enjoyer to create a PR to this repo so I had to fix the CI to trigger on pull_requests and not just push. Could you please push whatever to this branch to trigger the CI (hopefully). I think just a

git commit --amend --no-edit

will rehash the last commit and trigger it too.

@lukamac lukamac changed the title Pr/nopmsis Make dependency on pmsis optional and add more parameters to Neureka Apr 10, 2025
@da-gazzi
Copy link
Author

@da-gazzi you are the first fork enjoyer to create a PR to this repo so I had to fix the CI to trigger on pull_requests and not just push. Could you please push whatever to this branch to trigger the CI (hopefully). I think just a

git commit --amend --no-edit

will rehash the last commit and trigger it too.

Thanks! I think it worked, I see new actions :D

@lukamac
Copy link
Collaborator

lukamac commented Apr 10, 2025

@da-gazzi the linting I believe you can solve by yourself, but the test errors I can take upon myself. Either I write the fixes here, maybe if they are not too big, or you give me permission on your fork to commit directly there

@da-gazzi
Copy link
Author

Lint errors should be fixed, and Moritz has given you access to his fork - thanks!

@lukamac lukamac force-pushed the pr/nopmsis branch 2 times, most recently from 5abcbd2 to 83f8b40 Compare April 11, 2025 07:39
@lukamac
Copy link
Collaborator

lukamac commented Apr 11, 2025

Your changes -> my changes (I edited the first commit and force pushed which made that commit and all further commits mine, honest mistake 👀 )

I think I understand the idea behind making pulp-nnx an INTERFACE, and I agree it would be saner but how we currently add the pulp-sdk dependency to it is a bit jank. Instead of having it as a dependency of the BSP it is actually linked to the pulp-nnx library in the user's CMakeLists.txt (example).
It's not a pretty solution, but I feel like doing it properly would require significant effort.
I'm proposing an intermediate solution where we keep the addition of a new library pulp-nnx-hal which is bsp-less, but keep the pulp-nnx as it was, a statically linked library.

Is that good enough for your use case?

@da-gazzi
Copy link
Author

Hey Luka, sorry that I dropped the ball on this - the pr23-test branch is working fine on our end!

@lukamac
Copy link
Collaborator

lukamac commented May 8, 2025

closing this PR in favor of #26. All the original code preserved + added fixes

@lukamac lukamac closed this May 8, 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.

3 participants