feat: ble: add battery level characteristic #66
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
App might read battery level from characteristic/service uuid: 0x2A19/0x180F
Reviewer's Guide by SourceryThis pull request implements a new Battery Service profile in the Bluetooth Low Energy (BLE) stack, allowing a connected app to read the device's battery level. Sequence diagram for battery level reading via BLEsequenceDiagram
participant App as BLE App
participant Device as BLE Device
participant ADC as ADC Hardware
App->>Device: Read Battery Level (0x2A19)
Device->>ADC: Read battery voltage
ADC-->>Device: Return raw ADC value
Note over Device: Convert ADC value to percentage
Device-->>App: Return battery level percentage
Class diagram for battery service implementationclassDiagram
class BatteryService {
+ServiceUUID: 0x180F
+battLv_CharUUID: 0x2A19
+battLv_Val: uint8[]
+read_handler()
+batt_registerService()
}
class PowerManagement {
+batt_raw()
+charging_status()
+batt_raw2percent()
+poweroff()
}
BatteryService --> PowerManagement: uses
File-Level Changes
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 @kienvo - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider wrapping the ADC debug print statements in a debug macro/flag for production builds
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| LowPower_Shutdown(0); | ||
| } | ||
|
|
||
| int batt_raw() |
There was a problem hiding this comment.
suggestion (performance): Consider moving ADC calibration to an initialization function to avoid recalibrating on every battery reading
The ADC calibration value is unlikely to change during runtime. Caching it would improve performance and reduce power consumption.
Suggested implementation:
static int16_t adc_calib_value;
void power_init(void)
{
GPIOA_ModeCfg(GPIO_Pin_5, GPIO_ModeIN_Floating);
ADC_ExtSingleChSampInit(SampleFreq_3_2, ADC_PGA_0);
adc_calib_value = ADC_DataCalib_Rough();
PRINT("RoughCalib_Value = %d \n", adc_calib_value);
}
int batt_raw()
{
int ret = 0;
/* Use cached calibration value */
ADC_ChannelCfg(1);You'll also need to:
- Add a prototype for power_init() in the header file (likely power.h)
- Call power_init() during system initialization, before any battery readings are taken
- Consider adding error handling for the calibration process in power_init()
|
|
||
| ADC_ChannelCfg(1); | ||
|
|
||
| PRINT("ADC reading: \n"); |
There was a problem hiding this comment.
suggestion (performance): Debug print statements should be wrapped in conditional compilation
Consider using #ifdef DEBUG around PRINT statements to avoid unnecessary overhead in production builds
Suggested implementation:
int16_t adc_calib = ADC_DataCalib_Rough();
#ifdef DEBUG
PRINT("RoughCalib_Value = %d \n", adc_calib);
#endif
ADC_ChannelCfg(1);
#ifdef DEBUG
PRINT("ADC reading: \n");
#endif
uint16_t buf[20];
for(int i = 0; i < 20; i++) {
uint16_t adc = ADC_ExcutSingleConver() + adc_calib;
ret += adc;
#ifdef DEBUG
PRINT("%d \n", adc);
#endif
}Make sure that:
- DEBUG is defined in your debug build configuration (typically in a header file or build system)
- All other source files using PRINT statements are similarly updated for consistency
| } | ||
|
|
||
| *pLen = 1; | ||
| battLv_Val[0] = batt_raw2percent(batt_raw()); |
There was a problem hiding this comment.
issue (bug_risk): Battery level should be clamped to 0-100 range for BLE characteristic
batt_raw2percent can return negative values, but the Battery Level characteristic (0x2A19) must be between 0 and 100 per BLE spec
| return GPIOA_ReadPortPin(CHARGE_STT_PIN) == 0; | ||
| } | ||
|
|
||
| #define ZERO_PERCENT_THRES (3.3) |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the voltage calculation by using functions instead of macros and combining the voltage divider and ADC conversion into a single step.
The voltage calculation chain can be simplified while maintaining precision. Consider replacing the macro definitions and multi-step calculations with these more direct functions:
// Constants
#define BATT_MIN_VOLT 3.3 // Zero percent threshold
#define BATT_MAX_VOLT 4.2 // 100 percent threshold
#define ADC_MAX_VAL 4096.0
#define ADC_REF_VOLT 2.1
// Convert ADC reading directly to battery voltage
static inline float adc_to_battery_voltage(int adc_raw) {
// Combined voltage divider and ADC conversion
return adc_raw * (ADC_REF_VOLT / ADC_MAX_VAL) * (282.0 / 100.0);
}
// Convert battery voltage to percentage
static inline int battery_voltage_to_percent(float voltage) {
if (voltage < BATT_MIN_VOLT) return 0;
if (voltage > BATT_MAX_VOLT) return 100;
return (int)((voltage - BATT_MIN_VOLT) * 100.0 / (BATT_MAX_VOLT - BATT_MIN_VOLT));
}This approach:
- Eliminates the macro chain while preserving the calculations
- Makes the conversion steps clear and debuggable
- Maintains floating-point precision
- Keeps the electrical engineering concepts visible
The main function becomes simpler:
int batt_raw2percent(int r) {
return battery_voltage_to_percent(adc_to_battery_voltage(r));
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
App might read battery level from the standard characteristic/service uuid: 0x2A19/0x180F
Summary by Sourcery
New Features: