feat: enable always on mode#93
feat: enable always on mode#93samruddhi-Rahegaonkar wants to merge 3 commits intofossasia:masterfrom
Conversation
Reviewer's GuideThis PR adds a persistent BLE Always-On mode flag to badge configuration and wires it through configuration, BLE service, advertising logic, and power management, enabling live sync without entering low-power or stopping advertising. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @samruddhi-Rahegaonkar - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Top-level code outside of a function or block will cause a compilation error. (link)
General comments:
- In config.h you declare
bool ble_always_onbut never include <stdbool.h> and it duplicatesbadge_cfg.ble_always_on– drop the stray global and add the proper header for bool in config.h. - The reconnect handler in ng.c always restarts advertising whenever any connection status event occurs; move the Always-On re-advertise logic inside the block that handles a dropped link so it only triggers on actual disconnects.
- The status codes in enable/disable always-on functions invert the flash-write return value – double-check
cfg_writeflash_def’s return semantics and make sure success/fail are mapped correctly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In config.h you declare `bool ble_always_on` but never include <stdbool.h> and it duplicates `badge_cfg.ble_always_on` – drop the stray global and add the proper header for bool in config.h.
- The reconnect handler in ng.c always restarts advertising whenever any connection status event occurs; move the Always-On re-advertise logic inside the block that handles a dropped link so it only triggers on actual disconnects.
- The status codes in enable/disable always-on functions invert the flash-write return value – double-check `cfg_writeflash_def`’s return semantics and make sure success/fail are mapped correctly.
## Individual Comments
### Comment 1
<location> `src/ble/setup.c:43` </location>
<code_context>
+ && (!linkDB_Up(connHandle)))) {
+ GATTServApp_InitCharCfg(connHandle, TxCCCD);
+ }
+ if (badge_cfg.ble_always_on) {
+ PRINT("BLE disconnected, restarting advertising due to Always-On\n");
+ ble_enable_advertise();
</code_context>
<issue_to_address>
Top-level code outside of a function or block will cause a compilation error.
Move the badge_cfg.ble_always_on check and related function calls into an initialization function, such as main() or a BLE setup routine, to avoid compilation errors.
</issue_to_address>
### Comment 2
<location> `src/config.h:43` </location>
<code_context>
int cfg_writeflash_def(badge_cfg_t *cfg);
int cfg_readflash_def(badge_cfg_t *cfg);
void cfg_fallback();
+bool ble_always_on;
#endif /* __CONFIG_H__ */
</code_context>
<issue_to_address>
Global variable declaration in header may cause multiple definition errors.
Declare 'ble_always_on' as 'extern' in the header, and define it in one source file to avoid multiple definition errors.
</issue_to_address>
### Comment 3
<location> `src/usb/dev.c:94` </location>
<code_context>
- uint8_t index = request->wValue & 0xff;
- if (index <= sizeof(string_index))
- ctrl_start_load_block(string_index[index], string_index[index][0]);
+ static const void *string_index[4] = {
+ lang_desc,
+ vendor_info,
+ product_info,
+ serial_number
+ };
+
</code_context>
<issue_to_address>
Type of string_index changed to const void*, but usage may lead to type-punning and alignment issues.
Casting from const void* to uint8_t* or uint16_t* can cause undefined behavior due to type-punning and alignment issues. Use a union or struct for safe access, or retain uint8_t* and handle extraction carefully.
</issue_to_address>
### Comment 4
<location> `src/usb/dev.c:102` </location>
<code_context>
+ };
+
+ uint8_t index = request->wValue & 0xff;
+ if (index < 4) {
+ uint16_t length;
+ if (index == 0) {
+ length = ((const uint8_t *)string_index[index])[0];
+ } else {
+ length = ((const uint16_t *)string_index[index])[0] & 0xFF;
+ }
+ ctrl_start_load_block((const uint8_t *)string_index[index], length);
+ }
}
</code_context>
<issue_to_address>
String descriptor length extraction is inconsistent and may be incorrect for non-zero indices.
For index 0, length is read as uint8_t; for others, as uint16_t masked with 0xFF. This may misinterpret the descriptor length if it's not always in the first byte. Please ensure consistent descriptor parsing or refactor to handle varying formats correctly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
uint8_t index = request->wValue & 0xff;
if (index < 4) {
uint16_t length;
if (index == 0) {
length = ((const uint8_t *)string_index[index])[0];
} else {
length = ((const uint16_t *)string_index[index])[0] & 0xFF;
}
ctrl_start_load_block((const uint8_t *)string_index[index], length);
}
=======
uint8_t index = request->wValue & 0xff;
if (index < 4) {
// USB string descriptors always have the length in the first byte
uint16_t length = ((const uint8_t *)string_index[index])[0];
ctrl_start_load_block((const uint8_t *)string_index[index], length);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `src/legacyctrl.c:7` </location>
<code_context>
#include "debug.h"
#include "legacyctrl.h"
-int legacy_ble_rx(uint8_t *val, uint16_t len)
+void legacy_ble_rx(uint8_t *val, uint16_t len)
{
_TRACE();
</code_context>
<issue_to_address>
Return values removed from legacy_ble_rx, but error handling is now lost.
Previously, callers could handle errors based on return codes. With the function now void, errors may go unreported. Consider preserving error reporting, such as through a callback or global error state.
Suggested implementation:
```c
#include "debug.h"
#include "legacyctrl.h"
typedef enum {
LEGACY_BLE_RX_OK = 0,
LEGACY_BLE_RX_ERR_WIDTH,
LEGACY_BLE_RX_ERR_HEADER,
// Add more error codes as needed
} legacy_ble_rx_error_t;
static volatile legacy_ble_rx_error_t legacy_ble_rx_error = LEGACY_BLE_RX_OK;
void legacy_ble_rx(uint8_t *val, uint16_t len)
{
_TRACE();
static uint16_t c, data_len, n;
if (len != LEGACY_TRANSFER_WIDTH) {
PRINT("Transfer width is not matched\n");
legacy_ble_rx_error = LEGACY_BLE_RX_ERR_WIDTH;
return;
} else {
legacy_ble_rx_error = LEGACY_BLE_RX_OK;
}
PRINT("val[%d]: ", len);
if (c == 0) {
if (memcmp(val, "wang", 5)) {
legacy_ble_rx_error = LEGACY_BLE_RX_ERR_HEADER;
return;
}
```
- Update all error conditions in `legacy_ble_rx` to set `legacy_ble_rx_error` appropriately.
- If not already present, add a function to retrieve and clear the error state, e.g.:
`legacy_ble_rx_error_t legacy_ble_rx_get_last_error(void) { legacy_ble_rx_error_t err = legacy_ble_rx_error; legacy_ble_rx_error = LEGACY_BLE_RX_OK; return err; }`
- Update callers to check the global error state after calling `legacy_ble_rx`.
- Consider thread-safety if this code is used in a multi-threaded context.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (badge_cfg.ble_always_on) { | ||
| ble_enable_advertise(); | ||
| } else { |
There was a problem hiding this comment.
issue (bug_risk): Top-level code outside of a function or block will cause a compilation error.
Move the badge_cfg.ble_always_on check and related function calls into an initialization function, such as main() or a BLE setup routine, to avoid compilation errors.
| int cfg_writeflash_def(badge_cfg_t *cfg); | ||
| int cfg_readflash_def(badge_cfg_t *cfg); | ||
| void cfg_fallback(); | ||
| bool ble_always_on; |
There was a problem hiding this comment.
issue (bug_risk): Global variable declaration in header may cause multiple definition errors.
Declare 'ble_always_on' as 'extern' in the header, and define it in one source file to avoid multiple definition errors.
| static const void *string_index[4] = { | ||
| lang_desc, | ||
| vendor_info, | ||
| product_info, | ||
| serial_number |
There was a problem hiding this comment.
issue (bug_risk): Type of string_index changed to const void*, but usage may lead to type-punning and alignment issues.
Casting from const void* to uint8_t* or uint16_t* can cause undefined behavior due to type-punning and alignment issues. Use a union or struct for safe access, or retain uint8_t* and handle extraction carefully.
| uint8_t index = request->wValue & 0xff; | ||
| if (index < 4) { | ||
| uint16_t length; | ||
| if (index == 0) { | ||
| length = ((const uint8_t *)string_index[index])[0]; | ||
| } else { | ||
| length = ((const uint16_t *)string_index[index])[0] & 0xFF; | ||
| } | ||
| ctrl_start_load_block((const uint8_t *)string_index[index], length); | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): String descriptor length extraction is inconsistent and may be incorrect for non-zero indices.
For index 0, length is read as uint8_t; for others, as uint16_t masked with 0xFF. This may misinterpret the descriptor length if it's not always in the first byte. Please ensure consistent descriptor parsing or refactor to handle varying formats correctly.
| uint8_t index = request->wValue & 0xff; | |
| if (index < 4) { | |
| uint16_t length; | |
| if (index == 0) { | |
| length = ((const uint8_t *)string_index[index])[0]; | |
| } else { | |
| length = ((const uint16_t *)string_index[index])[0] & 0xFF; | |
| } | |
| ctrl_start_load_block((const uint8_t *)string_index[index], length); | |
| } | |
| uint8_t index = request->wValue & 0xff; | |
| if (index < 4) { | |
| // USB string descriptors always have the length in the first byte | |
| uint16_t length = ((const uint8_t *)string_index[index])[0]; | |
| ctrl_start_load_block((const uint8_t *)string_index[index], length); | |
| } |
| #include "debug.h" | ||
| #include "legacyctrl.h" | ||
|
|
||
| int legacy_ble_rx(uint8_t *val, uint16_t len) |
There was a problem hiding this comment.
suggestion (bug_risk): Return values removed from legacy_ble_rx, but error handling is now lost.
Previously, callers could handle errors based on return codes. With the function now void, errors may go unreported. Consider preserving error reporting, such as through a callback or global error state.
Suggested implementation:
#include "debug.h"
#include "legacyctrl.h"
typedef enum {
LEGACY_BLE_RX_OK = 0,
LEGACY_BLE_RX_ERR_WIDTH,
LEGACY_BLE_RX_ERR_HEADER,
// Add more error codes as needed
} legacy_ble_rx_error_t;
static volatile legacy_ble_rx_error_t legacy_ble_rx_error = LEGACY_BLE_RX_OK;
void legacy_ble_rx(uint8_t *val, uint16_t len)
{
_TRACE();
static uint16_t c, data_len, n;
if (len != LEGACY_TRANSFER_WIDTH) {
PRINT("Transfer width is not matched\n");
legacy_ble_rx_error = LEGACY_BLE_RX_ERR_WIDTH;
return;
} else {
legacy_ble_rx_error = LEGACY_BLE_RX_OK;
}
PRINT("val[%d]: ", len);
if (c == 0) {
if (memcmp(val, "wang", 5)) {
legacy_ble_rx_error = LEGACY_BLE_RX_ERR_HEADER;
return;
}- Update all error conditions in
legacy_ble_rxto setlegacy_ble_rx_errorappropriately. - If not already present, add a function to retrieve and clear the error state, e.g.:
legacy_ble_rx_error_t legacy_ble_rx_get_last_error(void) { legacy_ble_rx_error_t err = legacy_ble_rx_error; legacy_ble_rx_error = LEGACY_BLE_RX_OK; return err; } - Update callers to check the global error state after calling
legacy_ble_rx. - Consider thread-safety if this code is used in a multi-threaded context.
There was a problem hiding this comment.
@samruddhi-Rahegaonkar Do you even know what you are doing? This PR is not acceptable. The whitespace changes are still there.
The CI build is failing. Did you ever set up the toolchain to build this mess? Or did you ever actually test this code on real hardware?
I assume that you just throw this repo to an LLM and push this mess here. There are a lot of nonsense changes here. Firmware development is not something LLMs can handle. Don't do this please.
|
@kienvo I’m still relatively new to firmware development, and I’m actively learning and improving with each iteration. I understand the importance of clean code and proper testing, and I’ll work on addressing the issues you mentioned. |
|
@kienvo I just want to clarify that I created this PR as a draft specifically to get early input and guidance from you while I'm still actively working on the changes. I’m aware there are issues like build errors and formatting, and I’m in the process of resolving them. Firmware development is new territory for me, and I understand that it's not easy to get everything perfect in one go. I’ve already set up the toolchain and managed to build the project successfully once. I'm not just relying on an LLM — I’m trying to learn by doing, and by taking feedback like yours seriously. |
2 participants
This PR implements the BLE Always-On mode for the badge. When enabled, the badge will not enter low-power state and BLE advertising remains active for live sync, even if poweroff is triggered.
Please review and suggest improvements!
Summary by Sourcery
Enable a new BLE Always-On mode that prevents low-power shutdown and keeps BLE advertising active for live sync, with commands to toggle and persist the setting
New Features:
Enhancements: