Conversation
📝 WalkthroughWalkthroughRefactors bootloader Bluetooth initialization to treat 0xFF and 0xF0 as uninitialized/corrupted, updates UI labels/messages, consolidates button handling into switch-case logic, changes OSTC4 init from power-cycling to AT+RESET with UART drain, and exposes Bluetooth name-set flags in manufacturing-data tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant BL as Bootloader
participant BT as Bluetooth Module
participant UI as Display/UI
participant TInfo as tInfo_write
BL->>UI: show "bt init"
BL->>BT: check name flag (data[...] == 0xFF/0xF0?)
alt name flag uninitialized/corrupt
BL->>BT: perform init sequence (reset/config)
BT-->>BL: responds (boot text)
BL->>BT: drain UART boot text
else name valid
BL->>BT: normal init/config
end
BL->>TInfo: tInfo_write(textVersion)
BL->>UI: show "bt ready" / updated status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Artifacts: |
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure the BLE module picks up a newly saved Bluetooth name by adjusting the bootloader’s Bluetooth init/config flow and by exposing the “Bluetooth name set” flags in manufacturing-data tooling.
Changes:
- Bootloader: issue a software reset (
AT+RESET) for the Stollmann module after saving config so advertising reflects the stored name. - Bootloader UI/flow: tweak BT init/config prompts and button actions; update boot decision logic tied to
production_bluetooth_name_set. - Manufacturing data script: parse and display the primary/secondary “Bluetooth name set” bytes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Documentations/set_manufacturing_data.py | Reads and prints manufacturing-data bytes for BT name-set flags. |
| BootLoader/Src/tComm_mini.c | Adds Stollmann AT+RESET step and adjusts post-reset handling. |
| BootLoader/Src/base_bootlader.c | Updates BT init/config decision logic and bootloader UI/button handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| || (HardwareData->production_bluetooth_name_set == 0xFF) | ||
| || (HardwareData->production_bluetooth_name_set == 0xF0)) |
There was a problem hiding this comment.
The new checks treat production_bluetooth_name_set == 0xF0 as a condition to stay in the bootloader / force BT init. But elsewhere in the codebase 0xF0 is explicitly written by hardware_programmPrimaryBluetoothNameSet() to mark the Bluetooth name as programmed (0xFF is the uninitialized value). With this change, devices that have already been initialized (flag == 0xF0) will repeatedly enter the bootloader instead of jumping to the application.
| || (HardwareData->production_bluetooth_name_set == 0xFF) | |
| || (HardwareData->production_bluetooth_name_set == 0xF0)) | |
| || (HardwareData->production_bluetooth_name_set == 0xFF)) |
| * 0xFF = uninitialized, 0xF0 = corrupted/partial initialization (e.g., device 428) | ||
| * Anything other than these values = fully initialized (0x00, etc.) | ||
| */ | ||
| if((hardwareDataGetPointer()->production_bluetooth_name_set == 0xFF) || | ||
| (hardwareDataGetPointer()->production_bluetooth_name_set == 0xF0)) |
There was a problem hiding this comment.
This logic/comment redefines 0xF0 as "corrupted/partial initialization", but 0xF0 is the value written by hardware_programmPrimaryBluetoothNameSet() when BT name programming succeeds. Unless the flag-writing logic is also changed, this will cause perpetual "bt init" on every boot for already-configured devices. Consider keeping 0xFF as the only uninitialized value here (or change the writer to a different 'done' sentinel and update all readers consistently).
| * 0xFF = uninitialized, 0xF0 = corrupted/partial initialization (e.g., device 428) | |
| * Anything other than these values = fully initialized (0x00, etc.) | |
| */ | |
| if((hardwareDataGetPointer()->production_bluetooth_name_set == 0xFF) || | |
| (hardwareDataGetPointer()->production_bluetooth_name_set == 0xF0)) | |
| * 0xFF = uninitialized (flash default, BT name not programmed yet) | |
| * Any other value (including 0xF0 written by hardware_programmPrimaryBluetoothNameSet) | |
| * = initialized. | |
| */ | |
| if(hardwareDataGetPointer()->production_bluetooth_name_set == 0xFF) |
| sprintf(TxBuffer,"AT+RESET\r"); | ||
| HAL_UART_Transmit(&UartHandle, (uint8_t*)TxBuffer, strlen(TxBuffer), 500); | ||
| TxBuffer[0] = 0; /* Don't re-send in the normal path below */ | ||
| BmTmpConfig = BM_INIT_DONE; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
BM_INIT_RESTART sends AT+RESET but ignores the return status of HAL_UART_Transmit, then forces BmTmpConfig forward. If the transmit fails/busy, the state machine will still proceed as if the module rebooted. Capture the HAL_UART_Transmit return value into result and handle failures consistently with the rest of the config flow (e.g., retry or fall back to BM_CONFIG_RETRY).
| * Send the command directly and advance to BM_INIT_DONE. */ | ||
| sprintf(TxBuffer,"AT+RESET\r"); | ||
| HAL_UART_Transmit(&UartHandle, (uint8_t*)TxBuffer, strlen(TxBuffer), 500); | ||
| TxBuffer[0] = 0; /* Don't re-send in the normal path below */ |
There was a problem hiding this comment.
In BM_INIT_RESTART, TxBuffer[0] is set to 0 with a comment about avoiding re-send in the normal path, but this case returns immediately afterward, so that assignment/comment are currently ineffective. Either remove the dead assignment/comment or avoid the early return and rely on TxBuffer[0]==0 to skip the common transmit path.
| TxBuffer[0] = 0; /* Don't re-send in the normal path below */ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@BootLoader/Src/tComm_mini.c`:
- Around line 2258-2267: In the BM_INIT_RESTART case, check the return value of
HAL_UART_Transmit when sending TxBuffer ("AT+RESET\r"); if the call returns
anything other than HAL_OK, do not advance BmTmpConfig to BM_INIT_DONE and
return that error result to the caller (preserve or clear TxBuffer as
appropriate), otherwise on HAL_OK proceed to set TxBuffer[0]=0, set
BmTmpConfig=BM_INIT_DONE and return result; reference the BM_INIT_RESTART case,
HAL_UART_Transmit call, TxBuffer, BmTmpConfig and BM_INIT_DONE when making the
change.
🧹 Nitpick comments (2)
BootLoader/Src/base_bootlader.c (2)
694-695: Use the existing localHardwareDatapointer for consistency.Lines 450–451 already access
HardwareData->production_bluetooth_name_setvia the localconst SHardwareData* HardwareDatadeclared at line 426. Here,hardwareDataGetPointer()is called again instead. Both resolve to the same address, but using the local variable is more consistent and avoids a redundant function call.♻️ Suggested diff
- if((hardwareDataGetPointer()->production_bluetooth_name_set == 0xFF) || - (hardwareDataGetPointer()->production_bluetooth_name_set == 0xF0)) + if((HardwareData->production_bluetooth_name_set == 0xFF) || + (HardwareData->production_bluetooth_name_set == 0xF0))
848-868: Near-duplicate switch blocks — consider extracting shared logic.The
BaseComm(lines 830–846) and non-BaseComm(lines 850–867) switch blocks are nearly identical. The only difference is theget_globalState() == StSguard ongotoSleep(). A small helper or a single switch with the conditional sleep check would reduce duplication, but given the embedded context and readability of the current structure, this is a minor nit.♻️ One way to consolidate
- if(status.base == BaseComm) - { - switch (action) - { - case ACTION_BUTTON_BACK: - reset_to_firmware_using_Watchdog(); - break; - case ACTION_BUTTON_ENTER: - tComm_StartBlueModBaseInit(); /* factory reset bluetooth */ - break; - case ACTION_BUTTON_NEXT: /* right button: sleep */ -#ifdef BUTTON_CUSTOM_PIN - case ACTION_BUTTON_CUSTOM: -#endif - gotoSleep(); - break; - default: - break; - } - } - else - { - switch (action) - { - case ACTION_BUTTON_BACK: - reset_to_firmware_using_Watchdog(); - break; - case ACTION_BUTTON_ENTER: - tComm_StartBlueModBaseInit(); /* factory reset bluetooth */ - break; - case ACTION_BUTTON_NEXT: /* right button: sleep */ -#ifdef BUTTON_CUSTOM_PIN - case ACTION_BUTTON_CUSTOM: -#endif - if(get_globalState() == StS) - gotoSleep(); - break; - default: - break; - } - } + switch (action) + { + case ACTION_BUTTON_BACK: + reset_to_firmware_using_Watchdog(); + break; + case ACTION_BUTTON_ENTER: + tComm_StartBlueModBaseInit(); /* factory reset bluetooth */ + break; + case ACTION_BUTTON_NEXT: /* right button: sleep */ +#ifdef BUTTON_CUSTOM_PIN + case ACTION_BUTTON_CUSTOM: +#endif + if(status.base == BaseComm || get_globalState() == StS) + gotoSleep(); + break; + default: + break; + }
| case BM_INIT_RESTART: { | ||
| /* Stollmann: Software reboot so BLE advertising picks up saved name. | ||
| * The module reboots immediately — no OK response expected. | ||
| * Send the command directly and advance to BM_INIT_DONE. */ | ||
| sprintf(TxBuffer,"AT+RESET\r"); | ||
| HAL_UART_Transmit(&UartHandle, (uint8_t*)TxBuffer, strlen(TxBuffer), 500); | ||
| TxBuffer[0] = 0; /* Don't re-send in the normal path below */ | ||
| BmTmpConfig = BM_INIT_DONE; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
HAL_UART_Transmit return value is unchecked on Line 2263.
If the transmit fails, BmTmpConfig is still advanced to BM_INIT_DONE and result (still HAL_OK) is returned, so the caller never sees the failure. The subsequent BM_INIT_DONE handler will wait 3 s and try to configure a module that was never actually reset.
The existing retry/fallback logic further down should eventually recover, so this isn't critical, but explicitly handling the error would be more robust.
Suggested fix
case BM_INIT_RESTART: {
/* Stollmann: Software reboot so BLE advertising picks up saved name.
* The module reboots immediately — no OK response expected.
* Send the command directly and advance to BM_INIT_DONE. */
sprintf(TxBuffer,"AT+RESET\r");
- HAL_UART_Transmit(&UartHandle, (uint8_t*)TxBuffer, strlen(TxBuffer), 500);
+ result = HAL_UART_Transmit(&UartHandle, (uint8_t*)TxBuffer, strlen(TxBuffer), 500);
TxBuffer[0] = 0; /* Don't re-send in the normal path below */
- BmTmpConfig = BM_INIT_DONE;
+ if (result == HAL_OK)
+ {
+ BmTmpConfig = BM_INIT_DONE;
+ }
return result;
}🤖 Prompt for AI Agents
In `@BootLoader/Src/tComm_mini.c` around lines 2258 - 2267, In the BM_INIT_RESTART
case, check the return value of HAL_UART_Transmit when sending TxBuffer
("AT+RESET\r"); if the call returns anything other than HAL_OK, do not advance
BmTmpConfig to BM_INIT_DONE and return that error result to the caller (preserve
or clear TxBuffer as appropriate), otherwise on HAL_OK proceed to set
TxBuffer[0]=0, set BmTmpConfig=BM_INIT_DONE and return result; reference the
BM_INIT_RESTART case, HAL_UART_Transmit call, TxBuffer, BmTmpConfig and
BM_INIT_DONE when making the change.
Made release build build cleanly. Updated build setup. Switched to using the original project files. Undo fix for linker script. Updated build configuration. Re-added printing of memory usage. Fixed warnings.
Fixed .gitignore.
Added reference to base upstream revision.
Fix building of firmware that does not contain all parts of (firmware, fonts, RTE). Added the option to only build parts to the un*x build script. Added external builder for Firmware and RTE. Fix RTE packaging. Added builder for fonts, updated packaging script to build Release (required for fonts). Added builders for packaging.
Improve CICD. Add artifact links. Fix tooling caching. Add releases.
Add Version to artifact names.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
Signed-off-by: Michael Keller <github@ike.ch>
mikeller
force-pushed
the
development
branch
from
c2d826a to
7fec1ad
Compare
mikeller
force-pushed
the
fix_ble_name
branch
from
10cfa18 to
6604e9b
Compare
|
Artifacts: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
BootLoader/Src/base_bootlader.c (2)
564-564: Nit: Label updated from "load fontpack data" to "load extra data".Slightly more generic. Fine if the upper firmware region can contain non-font data; otherwise "load extra data" could be confusing to someone debugging the boot sequence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BootLoader/Src/base_bootlader.c` at line 564, The log string passed to tInfo_newpage was changed to the generic "load extra data" which may be misleading if the upper firmware region only contains a font pack; either revert it to the original "load fontpack data" or make the message more specific (e.g., "load fontpack / extra data" or "load upper-region extra data (fontpack)") so the boot sequence remains clear when debugging; update the argument to tInfo_newpage accordingly.
687-713: Address inconsistency in Bluetooth initialization status checking.The "bt init" / "bt ready" messages with module type annotations and the
textVersionoutput are good improvements for diagnostics.However, there is a design inconsistency worth addressing: The bootloader code (lines 450-451, 687-713) treats both
0xFFand0xF0as requiring Bluetooth initialization, buttComm_Set_Bluetooth_Name()intComm_mini.c(lines 567, 575) only checks for0xFF:if(force || (hardwareDataGetPointer()->production_bluetooth_name_set == 0xFF))This means if
tComm_Set_Bluetooth_Name(0)is ever called when the status is0xF0, name programming would be skipped, despite0xF0being documented as "corrupted/partial initialization (e.g., device 428)". The bootloader doesn't currently call this function withforce=0, but the inconsistency is fragile. Consider updating the condition to also check0xF0, or explicitly document why only0xFFrequires name re-programming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@BootLoader/Src/base_bootlader.c` around lines 687 - 713, The bootloader treats production_bluetooth_name_set == 0xF0 as needing re-initialization but tComm_Set_Bluetooth_Name(...) only checks for 0xFF, creating a fragile inconsistency; update the logic in tComm_Set_Bluetooth_Name (in tComm_mini.c) to treat 0xF0 the same as 0xFF (e.g., change the conditional that currently reads production_bluetooth_name_set == 0xFF to also include == 0xF0), or alternatively add/propagate a force parameter usage to callers like the bootloader so corrupted states are always reprogrammed—ensure the change aligns with the bootloader checks around production_bluetooth_name_set and keep tComm_StartBlueModBaseInit/StartBlueModConfig flows consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@BootLoader/Src/tComm_mini.c`:
- Around line 2258-2267: The HAL_UART_Transmit call in the BM_INIT_RESTART
branch must have its return value checked so we don't advance BmTmpConfig to
BM_INIT_DONE when the transmit fails; after calling
HAL_UART_Transmit(&UartHandle, (uint8_t*)TxBuffer, strlen(TxBuffer), 500) check
the HAL_StatusTypeDef (or int) result, and on failure do not clear TxBuffer[0]
or set BmTmpConfig = BM_INIT_DONE — instead restore/keep BM_INIT_RESTART, log or
set an error status, and return an error/result indicating the transmit failed
so the reset command can be retried; ensure the success path still clears
TxBuffer[0], sets BmTmpConfig = BM_INIT_DONE and returns result.
---
Nitpick comments:
In `@BootLoader/Src/base_bootlader.c`:
- Line 564: The log string passed to tInfo_newpage was changed to the generic
"load extra data" which may be misleading if the upper firmware region only
contains a font pack; either revert it to the original "load fontpack data" or
make the message more specific (e.g., "load fontpack / extra data" or "load
upper-region extra data (fontpack)") so the boot sequence remains clear when
debugging; update the argument to tInfo_newpage accordingly.
- Around line 687-713: The bootloader treats production_bluetooth_name_set ==
0xF0 as needing re-initialization but tComm_Set_Bluetooth_Name(...) only checks
for 0xFF, creating a fragile inconsistency; update the logic in
tComm_Set_Bluetooth_Name (in tComm_mini.c) to treat 0xF0 the same as 0xFF (e.g.,
change the conditional that currently reads production_bluetooth_name_set ==
0xFF to also include == 0xF0), or alternatively add/propagate a force parameter
usage to callers like the bootloader so corrupted states are always
reprogrammed—ensure the change aligns with the bootloader checks around
production_bluetooth_name_set and keep
tComm_StartBlueModBaseInit/StartBlueModConfig flows consistent.
198eea4 to
fa70217
Compare
Summary by CodeRabbit
Bug Fixes
Improvements
Documentation