From f49d482ccd4be46a9480a54a4cacc56396c1a5bc Mon Sep 17 00:00:00 2001 From: Laurens Valk Date: Mon, 10 Feb 2025 10:28:37 +0100 Subject: [PATCH 1/4] pbio/drv/charger: Move charge enable logic. The charger module should be in charge of charging, not the battery module. This ensures that the battery status getter is synchronized with the actual battery state. Before this commit, the battery could be charging before it was reflected in the charging state. This will also give us more control to disable charging under certain charger error state conditions in the next commit. --- lib/pbio/drv/charger/charger_mp2639a.c | 44 +++++++++++++++++++++++--- lib/pbio/include/pbdrv/charger.h | 10 ------ lib/pbio/sys/battery.c | 26 --------------- 3 files changed, 39 insertions(+), 41 deletions(-) diff --git a/lib/pbio/drv/charger/charger_mp2639a.c b/lib/pbio/drv/charger/charger_mp2639a.c index 259d43f9d..ae2cfbf3e 100644 --- a/lib/pbio/drv/charger/charger_mp2639a.c +++ b/lib/pbio/drv/charger/charger_mp2639a.c @@ -22,12 +22,9 @@ #include #include #include -#if PBDRV_CONFIG_CHARGER_MP2639A_MODE_PWM | PBDRV_CONFIG_CHARGER_MP2639A_ISET_PWM #include -#endif -#if PBDRV_CONFIG_CHARGER_MP2639A_CHG_RESISTOR_LADDER #include -#endif +#include #include #include @@ -70,7 +67,13 @@ pbdrv_charger_status_t pbdrv_charger_get_status(void) { return pbdrv_charger_status; } -void pbdrv_charger_enable(bool enable, pbdrv_charger_limit_t limit) { +/** + * Enables or disables the charger. + * + * @param enable True to enable the charger, false to disable. + * @param limit The current limit to set. Only used on some platforms. + */ +static void pbdrv_charger_enable(bool enable, pbdrv_charger_limit_t limit) { #if PBDRV_CONFIG_CHARGER_MP2639A_ISET_PWM // Set the current limit (ISET) based on the type of charger attached. @@ -116,6 +119,32 @@ void pbdrv_charger_enable(bool enable, pbdrv_charger_limit_t limit) { mode_pin_is_low = enable; } +/** + * Enables the charger if USB is connected, otherwise disables charger. + */ +static void pbdrv_charger_enable_if_usb_connected(void) { + pbdrv_usb_bcd_t bcd = pbdrv_usb_get_bcd(); + bool enable = bcd != PBDRV_USB_BCD_NONE; + pbdrv_charger_limit_t limit; + + // This battery charger chip will automatically monitor VBUS and + // limit the current if the VBUS voltage starts to drop, so these limits + // are a bit looser than they could be. + switch (bcd) { + case PBDRV_USB_BCD_NONE: + limit = PBDRV_CHARGER_LIMIT_NONE; + break; + case PBDRV_USB_BCD_STANDARD_DOWNSTREAM: + limit = PBDRV_CHARGER_LIMIT_STD_MAX; + break; + default: + limit = PBDRV_CHARGER_LIMIT_CHARGING; + break; + } + + pbdrv_charger_enable(enable, limit); +} + /** * Gets the current CHG signal status (inverted compared to /CHG pin state). */ @@ -174,6 +203,11 @@ PROCESS_THREAD(pbdrv_charger_mp2639a_process, ev, data) { PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer)); etimer_restart(&timer); + // Enable charger chip based on USB state. We don't need to disable it + // on charger fault since the chip will automatically disable itself. + // If we disable it here we can't detect the fault condition. + pbdrv_charger_enable_if_usb_connected(); + chg_samples[chg_index] = read_chg(); if (mode_pin_is_low) { diff --git a/lib/pbio/include/pbdrv/charger.h b/lib/pbio/include/pbdrv/charger.h index 1c8a4a154..c0bc285a3 100644 --- a/lib/pbio/include/pbdrv/charger.h +++ b/lib/pbio/include/pbdrv/charger.h @@ -72,13 +72,6 @@ pbio_error_t pbdrv_charger_get_current_now(uint16_t *current); */ pbdrv_charger_status_t pbdrv_charger_get_status(void); -/** - * Enables or disables charging. - * @param [in] enable True to enable charging or false for discharging. - * @param [in] limit The current limit for the charging rate. - */ -void pbdrv_charger_enable(bool enable, pbdrv_charger_limit_t limit); - #else static inline pbio_error_t pbdrv_charger_get_current_now(uint16_t *current) { @@ -90,9 +83,6 @@ static inline pbdrv_charger_status_t pbdrv_charger_get_status(void) { return PBDRV_CHARGER_STATUS_FAULT; } -static inline void pbdrv_charger_enable(bool enable, pbdrv_charger_limit_t limit) { -} - #endif #endif // _PBDRV_CHARGER_H_ diff --git a/lib/pbio/sys/battery.c b/lib/pbio/sys/battery.c index cfe7581f1..37d75aa0e 100644 --- a/lib/pbio/sys/battery.c +++ b/lib/pbio/sys/battery.c @@ -99,32 +99,6 @@ void pbsys_battery_poll(void) { } else if (avg_battery_voltage >= battery_ok_mv) { pbsys_status_clear(PBIO_PYBRICKS_STATUS_BATTERY_LOW_VOLTAGE_WARNING); } - - // REVISIT: we should be able to make this event driven rather than polled - #if PBDRV_CONFIG_CHARGER - - pbdrv_usb_bcd_t bcd = pbdrv_usb_get_bcd(); - bool enable = bcd != PBDRV_USB_BCD_NONE; - pbdrv_charger_limit_t limit; - - // REVISIT: The only current battery charger chip will automatically monitor - // VBUS and limit the current if the VBUS voltage starts to drop, so these - // limits are a bit looser than they could be. - switch (bcd) { - case PBDRV_USB_BCD_NONE: - limit = PBDRV_CHARGER_LIMIT_NONE; - break; - case PBDRV_USB_BCD_STANDARD_DOWNSTREAM: - limit = PBDRV_CHARGER_LIMIT_STD_MAX; - break; - default: - limit = PBDRV_CHARGER_LIMIT_CHARGING; - break; - } - - pbdrv_charger_enable(enable, limit); - - #endif // PBDRV_CONFIG_CHARGER } /** From 78d438354304dbd42b03f4565d965f378e76823f Mon Sep 17 00:00:00 2001 From: Laurens Valk Date: Mon, 10 Feb 2025 11:17:52 +0100 Subject: [PATCH 2/4] pbio/drv/charger: Add charge pause after timeout. After charging for a long time, we disable charging for some time. This matches observed behavior with the LEGO Education SPIKE V3.x firmware. --- lib/pbio/drv/charger/charger_mp2639a.c | 38 +++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/lib/pbio/drv/charger/charger_mp2639a.c b/lib/pbio/drv/charger/charger_mp2639a.c index ae2cfbf3e..98e5652e1 100644 --- a/lib/pbio/drv/charger/charger_mp2639a.c +++ b/lib/pbio/drv/charger/charger_mp2639a.c @@ -164,6 +164,24 @@ static bool read_chg(void) { #endif } +// Sample CHG signal at 4Hz to capture transitions to detect fault condition. +#define PBDRV_CHARGER_MP2639A_STATUS_SAMPLE_TIME (250) + +// After charging for a long time, we disable charging for some time. This +// matches observed behavior with the LEGO Education SPIKE V3.x firmware. +// +// Why? Due to the way the hardware works, the hub cannot be truly turned off +// while USB is plugged in. As a result, the charger is always on. For some +// battery-charger pairs, this causes the battery to stop charging normally in +// hardware when full as intended, automatically starting a new cycle after +// some time. But in some battery-charger pairs, the charger will reach a +// timeout state and not restart charging. When leaving such a combination +// plugged in overnight, it will time out and not be charged in the morning, +// which is not desirable. For this reason, we pause and restart charging +// manually if it has been plugged in for a long time. +#define PBDRV_CHARGER_MP2639A_CHARGE_TIMEOUT_MS (60 * 60 * 1000) +#define PBDRV_CHARGER_MP2639A_CHARGE_PAUSE_MS (30 * 1000) + PROCESS_THREAD(pbdrv_charger_mp2639a_process, ev, data) { PROCESS_BEGIN(); @@ -195,13 +213,11 @@ PROCESS_THREAD(pbdrv_charger_mp2639a_process, ev, data) { static bool chg_samples[7]; static uint8_t chg_index = 0; static struct etimer timer; - - // sample at 4Hz - etimer_set(&timer, 250); + static uint32_t charge_count = 0; for (;;) { + etimer_set(&timer, PBDRV_CHARGER_MP2639A_STATUS_SAMPLE_TIME); PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer)); - etimer_restart(&timer); // Enable charger chip based on USB state. We don't need to disable it // on charger fault since the chip will automatically disable itself. @@ -211,6 +227,10 @@ PROCESS_THREAD(pbdrv_charger_mp2639a_process, ev, data) { chg_samples[chg_index] = read_chg(); if (mode_pin_is_low) { + + // Keep track of how long we have been charging. + charge_count++; + // Count number of transitions seen during sampling window. int transitions = chg_samples[0] != chg_samples[PBIO_ARRAY_SIZE(chg_samples) - 1]; for (size_t i = 1; i < PBIO_ARRAY_SIZE(chg_samples); i++) { @@ -239,12 +259,22 @@ PROCESS_THREAD(pbdrv_charger_mp2639a_process, ev, data) { // devices) requires a momentary pulse on the /PB pin, which is // not wired up. pbdrv_charger_status = PBDRV_CHARGER_STATUS_DISCHARGE; + charge_count = 0; } // Increment sampling index with wrap around. if (++chg_index >= PBIO_ARRAY_SIZE(chg_samples)) { chg_index = 0; } + + // If we have been charging for a long time, pause charging for a while. + if (charge_count > (PBDRV_CHARGER_MP2639A_CHARGE_TIMEOUT_MS / PBDRV_CHARGER_MP2639A_STATUS_SAMPLE_TIME)) { + pbdrv_charger_status = PBDRV_CHARGER_STATUS_DISCHARGE; + pbdrv_charger_enable(false, PBDRV_CHARGER_LIMIT_NONE); + etimer_set(&timer, PBDRV_CHARGER_MP2639A_CHARGE_PAUSE_MS); + PROCESS_WAIT_EVENT_UNTIL(etimer_expired(&timer)); + charge_count = 0; + } } PROCESS_END(); From b2fd58d630e7af165248f5d3fbb4efbfe2773536 Mon Sep 17 00:00:00 2001 From: Laurens Valk Date: Mon, 10 Feb 2025 13:40:39 +0100 Subject: [PATCH 3/4] pbio/battery: Use a single averaging calculation. The one in pbsys did not converge and we should do this only once to reduce code size. Fixes https://github.com/pybricks/support/issues/2055 --- lib/pbio/src/battery.c | 4 +++ lib/pbio/sys/battery.c | 60 ++++++------------------------------------ 2 files changed, 12 insertions(+), 52 deletions(-) diff --git a/lib/pbio/src/battery.c b/lib/pbio/src/battery.c index 0dd7db868..991ee99a3 100644 --- a/lib/pbio/src/battery.c +++ b/lib/pbio/src/battery.c @@ -14,6 +14,10 @@ #include #include +#if !PBIO_CONFIG_MOTOR_PROCESS +#error "PBIO_CONFIG_MOTOR_PROCESS must be enabled to continously update battery voltage." +#endif + // Slow moving average battery voltage. static int32_t battery_voltage_avg_scaled; diff --git a/lib/pbio/sys/battery.c b/lib/pbio/sys/battery.c index 37d75aa0e..7f39840a5 100644 --- a/lib/pbio/sys/battery.c +++ b/lib/pbio/sys/battery.c @@ -7,15 +7,13 @@ // TODO: need to handle battery pack switch and Li-ion batteries for Technic Hub and NXT #include +#include #include #include #include #include #include -// period over which the battery voltage is averaged (in milliseconds) -#define BATTERY_PERIOD_MS 2500 - // These values are for Alkaline (AA/AAA) batteries #define BATTERY_OK_MV 6000 // 1.0V per cell #define BATTERY_LOW_MV 5400 // 0.9V per cell @@ -27,46 +25,7 @@ #define LIION_LOW_MV 6800 // 3.4V per cell #define LIION_CRITICAL_MV 6000 // 3.0V per cell -static uint32_t prev_poll_time; -static uint16_t avg_battery_voltage; - -#if PBDRV_CONFIG_BATTERY_ADC_TYPE == 1 -// special case to reduce code size on Move hub -#define battery_critical_mv BATTERY_CRITICAL_MV -#define battery_low_mv BATTERY_LOW_MV -#define battery_ok_mv BATTERY_OK_MV -#else -static uint16_t battery_critical_mv; -static uint16_t battery_low_mv; -static uint16_t battery_ok_mv; -#endif - -/** - * Initializes the system battery monitor. - */ void pbsys_battery_init(void) { - #if PBDRV_CONFIG_BATTERY_ADC_TYPE != 1 - pbdrv_battery_type_t type; - if (pbdrv_battery_get_type(&type) == PBIO_SUCCESS && type == PBDRV_BATTERY_TYPE_LIION) { - battery_critical_mv = LIION_CRITICAL_MV; - battery_low_mv = LIION_LOW_MV; - battery_ok_mv = LIION_OK_MV; - } else { - battery_critical_mv = BATTERY_CRITICAL_MV; - battery_low_mv = BATTERY_LOW_MV; - battery_ok_mv = BATTERY_OK_MV; - } - #endif - - pbdrv_battery_get_voltage_now(&avg_battery_voltage); - // This is mainly for the Technic Hub. It seems that the first battery voltage - // read is always low and causes the hub to shut down because of low battery - // voltage even though the battery isn't that low. - if (avg_battery_voltage < battery_critical_mv) { - avg_battery_voltage = battery_ok_mv; - } - - prev_poll_time = pbdrv_clock_get_ms(); } /** @@ -75,18 +34,15 @@ void pbsys_battery_init(void) { * This is called periodically to update the current battery state. */ void pbsys_battery_poll(void) { - uint32_t now; - uint32_t poll_interval; - uint16_t battery_voltage; - now = pbdrv_clock_get_ms(); - poll_interval = now - prev_poll_time; - prev_poll_time = now; + pbdrv_battery_type_t type; + bool is_liion = pbdrv_battery_get_type(&type) == PBIO_SUCCESS && type == PBDRV_BATTERY_TYPE_LIION; - pbdrv_battery_get_voltage_now(&battery_voltage); + uint32_t battery_critical_mv = is_liion ? LIION_CRITICAL_MV : BATTERY_CRITICAL_MV; + uint32_t battery_low_mv = is_liion ? LIION_LOW_MV : BATTERY_LOW_MV; + uint32_t battery_ok_mv = is_liion ? LIION_OK_MV : BATTERY_OK_MV; - avg_battery_voltage = (avg_battery_voltage * (BATTERY_PERIOD_MS - poll_interval) - + battery_voltage * poll_interval) / BATTERY_PERIOD_MS; + uint32_t avg_battery_voltage = pbio_battery_get_average_voltage(); if (avg_battery_voltage <= battery_critical_mv) { pbsys_status_set(PBIO_PYBRICKS_STATUS_BATTERY_LOW_VOLTAGE_SHUTDOWN); @@ -107,5 +63,5 @@ void pbsys_battery_poll(void) { * This is only valid on hubs with a built-in battery charger. */ bool pbsys_battery_is_full(void) { - return avg_battery_voltage >= LIION_FULL_MV; + return pbio_battery_get_average_voltage() >= LIION_FULL_MV; } From e876c5933653c66933f62021c5ebeb7718b29bd7 Mon Sep 17 00:00:00 2001 From: Laurens Valk Date: Thu, 13 Feb 2025 12:14:22 +0100 Subject: [PATCH 4/4] pbio/platform/prime_hub: Fix PWM channel for charge enable. --- lib/pbio/platform/prime_hub/platform.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pbio/platform/prime_hub/platform.c b/lib/pbio/platform/prime_hub/platform.c index 1acd3a0b0..25601b7bf 100644 --- a/lib/pbio/platform/prime_hub/platform.c +++ b/lib/pbio/platform/prime_hub/platform.c @@ -139,7 +139,7 @@ const pbdrv_bluetooth_btstack_platform_data_t pbdrv_bluetooth_btstack_platform_d const pbdrv_charger_mp2639a_platform_data_t pbdrv_charger_mp2639a_platform_data = { .mode_pwm_id = PWM_DEV_5_TLC5955, - .mode_pwm_ch = 15, + .mode_pwm_ch = 14, .chg_resistor_ladder_id = RESISTOR_LADDER_DEV_0, .chg_resistor_ladder_ch = PBDRV_RESISTOR_LADDER_CH_2, .ib_adc_ch = 3,