-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
esp8266: Add per-board configs, following other ports. #5118
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
Conversation
Inspired by #4991 |
ifeq ($(wildcard $(BOARD_DIR)/.),) | ||
$(error Invalid BOARD specified: $(BOARD_DIR)) | ||
endif | ||
|
||
include ../../py/mkenv.mk | ||
|
||
# qstr definitions (must come before including py.mk) |
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.
To match other ports, can you also add:
-include $(BOARD_DIR)/mpconfigboard.mk
Note the -
on the front to make it optional.
Although it won't be used for the generic board, it's possible another board might want it (e.g. to reference additional code outside the board dir (e.g. extmod) or to change compiler/linker flags).
02c31f4
to
e78d8d9
Compare
Failing when trying to load |
So, what are the most popular boards?
|
I think we can add those as seperate PRs, right? @jimmo Are you happy with this PR? Can you say it is reviewed? |
Looks good to me. I agree other boards can be added later. First thing to do would be convert the 512k build to a board. |
01e7b2c
to
f8f3e8e
Compare
Had a crack at adding the 512K board. A few things I'm not sure about a) Can board names start with a number? b) Not sure about these 3x in
It used to run after c) Should d) Should every |
Good question! Probably best not to. In this case it could be GENERIC_512K
Are they needed at all? What happens with these just removed?
Maybe moved to
Only add when first needed. |
@mcauser is this ready to review, or do you plan to tick all the boxes in the first post? |
@dpgeorge I’ll cleanup the 512k bits first then push the other tasks to separate PRs |
Hi @dpgeorge , Over to you for review. |
Not sure what increased the minimal build by 4 (bytes?). |
Did the board or MCU name change ? |
@nevercast on the 512K board it did.
Not sure how that impacts the minimal build though |
4fce96e
to
33336fc
Compare
Rebased off master, CI now passes 👌 |
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.
Great work! Just a few comments inline.
ports/esp8266/mpconfigport.h
Outdated
#define MICROPY_PERSISTENT_CODE_LOAD (1) | ||
#endif |
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.
I think with all these options that have guards added in this file, it'd be much simple to just not configure them at all here. Instead, the mpconfigboard.h file should enable the features it wants. So in GENERIC_512K/mpconfigboard.h it'd be pretty much empty. And in GENERIC/mpconfigboard.h it'd enable all the extra features.
Then this file here would be almost unchanged.
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.
That would bring it closer in line with the STM32 port, where features are off by default and enabled in board specific mpconfigboard.h's
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.
Updated as per your recommendations.
Defaults in /py/mpconfig.h
, with common features enabled in /ports/esp8266/mpconfigboard.h
, and board specific features enabled in /ports/esp8266/boards/GENERIC/mpconfigboard.h
.
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.
Looks good!
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.
In the STM32 port, there's an additional mpconfigboard_common.h
, which sets defaults for boards which do not provide enough config.
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.
That extra complexity isn't needed here, at least not now.
Defaults to BOARD=GENERIC.
Not sure why the 512k build fails... |
Looks like it has to do with the changes I suggested to esp8266_common.ld, removing build-*. Can you please revert that, ie use |
Undo, Abort, Retry |
Instead of disabling features in boards/GENERIC_512K
4b088bf
to
2941879
Compare
Rolled back changes to
|
|
Found an old ESP-01 with 512K flash (25Q40BT). Deployed
|
Nice work! |
Defaults to BOARD=GENERIC.
Makefile
mpconfigport_512k.h
- it's now in /boards/GENERIC_512KREADME.md
- 512KB FlashROM version.travis.yml
For later: