-
Notifications
You must be signed in to change notification settings - Fork 152
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
[BLE sample] Log sleep interval and encoding on boot #223
base: main
Are you sure you want to change the base?
[BLE sample] Log sleep interval and encoding on boot #223
Conversation
ryanrolds
commented
Nov 30, 2024
- Adding configuration logging to start-up
- Added more detail to the BLE project configuration
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.
Please use a more descriptive commit message title. Something like "[BLE sample] Log sleep interval and encoding on boot".
# The amount of time it sleeps between sending data. | ||
CONFIG_PRST_SLEEP_DURATION_MSEC=600000 | ||
|
||
# Choose one of the following encoding options: |
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 would refrain from exhaustively listing options in the prj.conf
file. Reasoning:
- Why list some configs and not others?
- Why only in
prj.conf
and notprj_debug.conf
? - More importantly, this list will go stale once we add or remove some of these options.
A quick example and pointing to Kconfig for details is enough, as before.
LOG_INF("Payload Encoding: BPARASITE_V2"); | ||
} | ||
|
||
LOG_INF("Sleep duration: %d", CONFIG_PRST_SLEEP_DURATION_MSEC); |
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.
Add unit " ms"
@@ -34,6 +34,17 @@ static int prst_loop(prst_sensors_t *sensors) { | |||
int main(void) { | |||
__ASSERT(!prst_init(), "Error in prst_init()"); | |||
prst_led_flash(2); | |||
|
|||
if (IS_ENABLED(CONFIG_PRST_BLE_ENCODING_BTHOME_V2)) { |
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.
This will silently output nothing when new encoding schemes are introduced. We have this information at compile time, we can also avoid paying the binary size cost for all these strings.
static void dump_config() {
#if IS_ENABLED(CONFIG_PRST_BLE_ENCODING_BTHOME_V2)
// Only capitalize first letter.
LOG_INFO("Payload encoding: BTHOME_V2");
#else if ...
...
#else
#error "Unhandled CONFIG_PRST_BLE_ENCODING_ choice in dump_config()"
#endif
LOG_INF("Sleep duration: %d ms", CONFIG_PRST_SLEEP_DURATION_MSEC);
}
int main(void) {
...
dump_config();
...
}
Thank you for the feedback. I will correct my PR in the next few days. |
This hasn't fallen off my rader. I've had a lot going on. |