-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add support for Teknic ClearCore #215
base: develop
Are you sure you want to change the base?
Conversation
CPPPATH=[ | ||
os.path.join(FRAMEWORK_DIR, "cores", "arduino", "api"), | ||
os.path.join(FRAMEWORK_DIR, "cores", "arduino"), | ||
os.path.join(FRAMEWORK_DIR, "variants", "clearcore"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this line because the code below it that checks for "is there a build.variant and if yes add it to the cpp path" already takes care of this:
platform-atmelsam/builder/frameworks/arduino/arduino-samd.py
Lines 214 to 221 in 1718730
if "build.variant" in board: | |
variants_dir = os.path.join( | |
"$PROJECT_DIR", board.get("build.variants_dir")) if board.get( | |
"build.variants_dir", "") else os.path.join(FRAMEWORK_DIR, "variants") | |
env.Append( | |
CPPPATH=[os.path.join(variants_dir, board.get("build.variant"))] | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, the only line I don't need is 191? Or also the two first arduino lines?
so, this would be the result:
CPPPATH=[
os.path.join(FRAMEWORK_DIR, "cores", "arduino", "api"),
os.path.join(FRAMEWORK_DIR, "cores", "arduino"),
os.path.join(FRAMEWORK_DIR, "Teknic", "LwIP", "LwIP", "port", "include"),
os.path.join(FRAMEWORK_DIR, "Teknic", "LwIP", "LwIP", "src", "include"),
os.path.join(FRAMEWORK_DIR, "Teknic", "libClearCore", "inc")
],
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, only os.path.join(FRAMEWORK_DIR, "variants", "clearcore"),
is not needed.
Also: Your changes can be automatically verified to be compiling successfully on all architectures if you add an environment for your board in one of the examples, e.g., to [env:clearcore]
platform = atmelsam
board = clearcore
framework = arduino This example gets auto-compiled in the Github CI (see |
Can you go in your repo's https://github.com/patrickwasp/platform-atmelsam/actions and click activate? You should get compile feedback then (because CI will only run here after someone approves it) |
I enabled it but the git action doesn't have |
It should automatically triger in each commit (after it's been enabled), so the next commit will trigger it. Something that'll come up in review is also that last empty line was removed (both |
Very nice, it built the clearcore firmware in the CI succesfully: https://github.com/patrickwasp/platform-atmelsam/actions/runs/6001903149/job/16277089103 I oversaw the missing newline end in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All requested changes made and CI passes.
Is there anything holding this PR back? Arduino just dropped support for its VSCode extension, so there's a bit more demand for this package to support the board. |
Closes #175.