-
Notifications
You must be signed in to change notification settings - Fork 2k
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
examples/gnrc_border_router: pass variables to docker #20618
base: master
Are you sure you want to change the base?
Conversation
This allows configuration of the app as described even when using `BUILD_IN_DOCKER=1`.
Ahh, that makes sense :) |
WIFI_PASS \ | ||
WIFI_SSID \ |
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.
Are those defined and documented anywhere in a generic way? grepping through the RIOT repo only shows them being mentioned in esp-common and atwinc15x0 driver documentation, where they are expected to be passed within CFLAGS anyway.
So instead of adding them here, I would use the same approach as for the other Makefile variables just for examples/gnrc_border_router
.
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.
They are actually not used by the example, but used by the only two WIFI drivers (ESP* and ATWINC15x0) we have.
They are documented separately in both drivers.
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'm not sure I follow. The example defines the environment variables in https://github.com/RIOT-OS/RIOT/blob/master/examples/gnrc_border_router/Makefile#L19-L20 and uses them in https://github.com/RIOT-OS/RIOT/blob/master/examples/gnrc_border_router/Makefile.wifi.conf to add them to CFLAGS
.
The documentation of both ESP* and the ATWINC15x0 explicitly adds them to CFLAGS
as well (e.g., https://github.com/RIOT-OS/RIOT/blob/master/cpu/esp32/doc.txt#L1639). The only other example I could find that uses these environment variables is examples/paho-mqtt
which also explicitly adds them to CFLAGS
later on: https://github.com/RIOT-OS/RIOT/blob/master/examples/paho-mqtt/Makefile#L62-L63.
So my point was that the environment variables themselves are not generically defined, but only custom to specific examples, and therefore I think those examples should add the respective DOCKER_ENV_VARS
themselves.
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.
@maribu ping
Might be nice to have this in before the hard feature freeze in two weeks from now.
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 guess the best would to actually move extending the CFLAGS
with the wifi password to a common place and documenting it. I have the feeling that every non-testing app that includes networking and will copy-paste that snipped anyway the moment someone starts using that app for a board with WiFi connectivity anyway.
Contribution description
This passes environment variables used to configure e.g. the uplink of the border router to the docker container, so that configuration works with
BUILD_IN_DOCKER=1
.Testing procedure
Run:
In
master
, you'll see "crazy" serial output, as the build system on the host expects normal stdio and WiFi as uplink, but the build system in the docker container expected slip as uplink and multiplexes the uplink with stdio.With this PR, the serial is no longer multiplexed with the uplink, but WiFi is used as configured.
Issues/PRs references
None