samples: sat-cont & sat-dual-stack: Add board support for nrf54l15tag and XIAO ESP32-C6 - #290
samples: sat-cont & sat-dual-stack: Add board support for nrf54l15tag and XIAO ESP32-C6#290Kr0nu5 wants to merge 1 commit into
Conversation
ceolin
left a comment
There was a problem hiding this comment.
Thanks a lot for the contribution, it is looking good. Just a few requests:
- Split the PR into two commits, one per board. It makes easier to bisect when debugging.
- Please provide a proper commit message, the body is empty, it is missing the signed-off needed by DCO. See https://github.com/HubbleNetwork/hubble-device-sdk?tab=contributing-ov-file#commit-guide
- The copyright in the file is saying Hubble, add your own.
| This will set the sat timer for 2 minutes instead | ||
| of until a satellite pass is overhead. | ||
|
|
||
| config EXTERNAL_ANTENNA |
There was a problem hiding this comment.
I don't think this is necessarily a sample an option, since it is a board property. It would be better to have it in the board Kconfig and apply it to any application using it (during the board initialization code).
Not a blocker, we can do it after ... but I think that is the right direction.
There was a problem hiding this comment.
Agreed - just found the board specific dir (port/esp-idf/hubblenetwork-sdk/boards/esp32-c6), would have put the options and code there if I saw if before. I will be sure to give the board option and antenna select function better naming to make clear it is for the XIAO ESP32-C6 as well.
| #else | ||
| ESP_LOGI(APP_TAG, "Using default (onboard) antenna"); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
This does not need to be duplicated in each sample if goes to the board implementation.
There was a problem hiding this comment.
Same as above - found the esp32-c6 board config dir and will move the definition there.
Thanks for the heads up - I will split the commits (with above changes) and be sure to add a proper commit message, sign-off, and copyright info. |
ceolin
left a comment
There was a problem hiding this comment.
Sorry for the late reply, we have been busy with the latest release :)
Thanks for following up on it. I think the NRF54 part could be done in a snippet, but I can do it later if you prefer. The Espressif part I don't think fits in the sample.
Can you have only the nrf54 part in the commit and merge that ?
| output-low; | ||
| line-name = "rf-switch-v2"; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
These overlays will need to be duplicated in all builds for this target, a better approach would be define either define a snippet for this or do it programmatically in the board initialization.
| ESP_LOGI(APP_TAG, "Using default (onboard) antenna"); | ||
| #endif | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think this code fits in the sample, the implementation it is supporting an external antenna only in a a hardcoded gpio ...
Overlay for nrf54l15tag selects antenna 1 by default but can be switched to antenna 2 or both in the overlay, while the config sets the tx power to the max (+7dbm). The XIAO board has an smd antenna it uses by default, so a kconfig.projbuild boolean
EXTERNAL_ANTENNAand accompanying code snippet were added to select the external antenna (u.fL) using the on-board RF switch.