Skip to content

Add Oxocards ports #8600

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

Merged
merged 5 commits into from
Nov 14, 2023
Merged

Add Oxocards ports #8600

merged 5 commits into from
Nov 14, 2023

Conversation

supcik
Copy link

@supcik supcik commented Nov 13, 2023

This change adds CircuitPython for the Oxocards boards based on ESP32 (https://oxocard.ch/en/).

I was not sure about the copyright notices in the source files. I left "Scott Shawcroft" and "Dan Halbert" in board.c and mpconfigboard.h. Let me know if I must put my name instead.

I also ran pre-commit run --all-files, but it changes many other ".c" and ".h" files. Is this normal, or do I have a problem with my configuration ? I also had many errors with uncrustify (such as circuitpython/tools/uncrustify.cfg:1058: Expected number , for 'indent_shift'; got 'false').

The CREATOR_ID and CREATION_ID are not yet registered. I plan to do it once the pull request is accepted.

Thank you in advance for your feedback.

@supcik
Copy link
Author

supcik commented Nov 13, 2023

PXL_20231113_113409221

Here is a picture of CircuitPython running on the Oxocard Connect.

@supcik
Copy link
Author

supcik commented Nov 14, 2023

I'm having a problem with the "uncrustify" of the "pre-commit".

It seems to run fine on my machine :

find ports/espressif/boards/oxo* -name mpconfigboard.h | xargs pre-commit run --files
Check Yaml...........................................(no files to check)Skipped
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
codespell................................................................Passed
Translations.............................................................Passed
Formatting...............................................................Passed

But it fails in the Github action.

Note that when the pre-commit changed my files, it also produced many warnings :

circuitpython/tools/uncrustify.cfg:411: option 'sp_type_question' is deprecated; did you want to use 'sp_before_ptr_star' instead?
circuitpython/tools/uncrustify.cfg:905: option 'sp_before_tr_emb_cmt' is deprecated; did you want to use 'sp_before_tr_cmt' instead?
circuitpython/tools/uncrustify.cfg:908: option 'sp_num_before_tr_emb_cmt' is deprecated; did you want to use 'sp_num_before_tr_cmt' instead?
Option<NUM>: at circuitpython/tools/uncrustify.cfg:1058: Expected number , for 'indent_shift'; got 'false'
circuitpython/tools/uncrustify.cfg:1124: option 'indent_sing_line_comments' is deprecated; did you want to use 'indent_single_line_comments_before' instead?
Option<NUM>: at circuitpython/tools/uncrustify.cfg:1205: Expected number , for 'indent_comma_paren'; got 'false'
Option<NUM>: at circuitpython/tools/uncrustify.cfg:1209: Expected number , for 'indent_bool_paren'; got 'false'
circuitpython/tools/uncrustify.cfg:2044: option 'nl_func_var_def_blk' is deprecated; it has been replaced by 'nl_var_def_blk_end_func_top'.
You can also use 'nl_var_def_blk_end' for additional functionality
Option<UNUM>: at circuitpython/tools/uncrustify.cfg:2538: Expected unsigned number , for 'align_nl_cont'; got 'false'
Option<UNUM>: at circuitpython/tools/uncrustify.cfg:2727: Expected unsigned number , for 'mod_full_brace_if_chain'; got 'false'
circuitpython/tools/uncrustify.cfg:2892: option 'pp_space' is deprecated; it has been replaced by 'pp_space_after'.
Option<NUM>: at circuitpython/tools/uncrustify.cfg:2947: Expected number , for 'pp_indent_brace'; got 'true'

I don't have experience with uncrustify (I'm rather using clang-format), but I will do some research on how to fix this.

I was using uncrustify version 0.77.1 and it does not work like
the version 0.72 used by Ununtu 22.04.
@supcik
Copy link
Author

supcik commented Nov 14, 2023

I have a potential explanation.

I am using a mac and I installed uncrustify using Homebrew. Homebrew installs the version 0.77.1_f.

The github actions uses the uncrustify from the Ubuntu packages and it installs the version 0.72

The versions 0.77.1_f and 0.72 do not produce the same result! I guess that the uncrustify configuration files on CircuitPython are not yet ready for the version 0.77.1. Should I open an issue for this ?

@supcik
Copy link
Author

supcik commented Nov 14, 2023

I had some interesting discussions with @jepler on Discord and I actually missed the information concerning the version of uncrustify in the doc. Sorry.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

This looks fine. As for copyright, you could change the files you change to be copyright by you, but it's not a requirement.

As discussed in discord, we use a specific version of uncrustify, with specific settings, to match up with what MicroPython uses. This prevents needless skew from upstream.

@dhalbert
Copy link
Collaborator

I won't merge immediately in case you want to change the copyright, but it doesn't really matter.

@supcik
Copy link
Author

supcik commented Nov 14, 2023

As discussed in discord, we use a specific version of uncrustify, with specific settings, to match up with what MicroPython uses. This prevents needless skew from upstream.

Thank you very much for the explanation. This makes the contribution somehow harder for Mac users, but it really makes sense. I might write a document to explain how to do it.

@supcik
Copy link
Author

supcik commented Nov 14, 2023

I won't merge immediately in case you want to change the copyright, but it doesn't really matter.

I don't need my name in the copyright, so feel free to merge the pull request whenever you want.

@tannewt
Copy link
Member

tannewt commented Nov 14, 2023

The CREATOR_ID and CREATION_ID are not yet registered. I plan to do it once the pull request is accepted.

Feel free to keep the creation ID listing in a repo you own so you don't need us to review it. Linking to it from here would be nice: https://github.com/creationid/creators

@tannewt tannewt merged commit 98a92ef into adafruit:main Nov 14, 2023
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