Skip to content
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

Add Battery Voltage Divider Driver #293

Merged
merged 12 commits into from Oct 27, 2020

Conversation

Nicell
Copy link
Member

@Nicell Nicell commented Oct 23, 2020

This is a big step towards having battery reporting over BLE. This driver implements a voltage divider meant for batteries using the Sensors API.

Feedback is welcome on the style/quality of my code!

@Nicell Nicell added core Core functionality/behavior of ZMK enhancement New feature or request labels Oct 23, 2020
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor comments.

app/boards/arm/bluemicro840/bluemicro840_v1_defconfig Outdated Show resolved Hide resolved
app/drivers/zephyr/battery_voltage_divider.c Show resolved Hide resolved

drv_data->as.resolution = 12;
#else
#error Unsupported ADC
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the whole battery_voltage_divider is dependend on ADC_NRFX_SAADC?
I don't understand why, it sounds like just having an ADC is enough, is there a specific functionality you rely on that I didn't get?

Copy link
Member Author

@Nicell Nicell Oct 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the reason for this is that the ADC channel config set up really depends on the chip we're using as well as the voltage divider configuration from my understanding. For now almost everyone is using nrf52 with a very specific voltage divider setup recommended from Nordic (hence the specific gain, reference, acquisition time, etc). The reason it's set up this way is so we can add more else ifs in the future to handle other chips. We could possibly make the gain/ref/acquisition options in the DT, but I'm not certain this is a good idea, and at this point we're all using the same set up.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation but I still do not get it:
The voltage divider comes from the device tree and is not dependent on the ADC itself. As for the ADC configuration, this should be generic. If another MCU is used, I would expect that the ADC reading would work out of the box with the configuration you set up. So I would just remove this #ifdef pattern.
But I may be missing something, so if you find it simpler/better to keep as is, all good :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the ifdef pattern would result in errors for non-nrf ADCs because I reference SAADC_CH_PSELP_PSELP_AnalogInput0, which is only for nrfx. This error is more explicit to someone who is trying to use a new MCU (with different ADC reqs).

Along with this, from my understanding other ADCs need different parts of the adc_channel_cfg defined compared to what nrf ADCs need. I'm personally not interested in researching every other MCU Zephyr supports to make a bunch of logic to handle each type. At the moment we only use nrf52, and every single one uses these specific gains, references, and acquisition times. A generic solution would be a nice enhancement to this in the future, but I don't think it's necessary to have this driver added to ZMK.

I personally couldn't find any generic samples that supported any ADC in Zephyr's repo. I believe the reasons for that are the same reasons I haven't created a generic solution. I could definitely be wrong about some of this though. I'm basing it off of what samples I could find and the source code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah sorry I missed the SAADC_CH_PSELP_PSELP_AnalogInput0 and indeed I see that the examples in zephyr repo use MCU specific gain/ref/acqu_time config.

@aakkds
Copy link

aakkds commented Oct 24, 2020

Hey @Nicell !
I took the liberty to join, hope you don't mind :)
That will be cool to have the battery reporting, I just have one "concern" related to the convertion mV -> %.
Basically, I don't think doing a complicated mapping to a discharge curve that does not correspond to the physical battery used is worth it.
I also added some minor stuff that I saw, which you can probably ignore ;)

@Nicell
Copy link
Member Author

Nicell commented Oct 24, 2020

@aakkds Thanks for the review! You're right it's probably not worth the newlib dependency for the more accurate function. I also addressed the NRFX ADC comment. Let me know if you think there's a better way to generalize this, but from what I've seen it's not that simple, and making a fully configurable ADC channel configuration system may overcomplicate defining the voltage divider for our use case.

drv_data->voltage = millivolts;
drv_data->state_of_charge = percent;
} else {
LOG_DBG("Failed to read ADC: %d", rc);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While first reading this I thought that it made sense to not return an error so that the power GPIO is still disabled.
This can have a bad side effect for the application calling the sensor_sample_fetch.
The call to sensor_sample_fetch will most likely succeed due to rc = gpio_pin_set(drv_data->gpio, drv_cfg->power_gpios.pin, 0); below, even if adc_read failed.

I think it is acceptable as the LOG_DBG at least will indicate something is wrong, but be aware that even if the application is doing error checking, this can get undetected.

If you want to take this into account, you could introduce a second variable rc_2 to do rc_2 = adc_read(...);
and in ligne 112 return (rc_2 != 0 ? rc_2 : rc);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should check for both. Probably easier to just have the later code in the disable block set int rc2 = gpio_pin_set(blah); and check that rc2 internally there and bail if it fails.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor things.

app/boards/arm/bluemicro840/bluemicro840_v1_defconfig Outdated Show resolved Hide resolved
app/boards/arm/nice_nano/nice_nano_defconfig Outdated Show resolved Hide resolved
app/boards/arm/nrfmicro/nrfmicro_13_defconfig Outdated Show resolved Hide resolved
drv_data->voltage = millivolts;
drv_data->state_of_charge = percent;
} else {
LOG_DBG("Failed to read ADC: %d", rc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should check for both. Probably easier to just have the later code in the disable block set int rc2 = gpio_pin_set(blah); and check that rc2 internally there and bail if it fails.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@petejohanson petejohanson merged commit 8c6862f into zmkfirmware:main Oct 27, 2020
1 check passed
MangoIV pushed a commit to MangoIV/zmk that referenced this pull request Dec 18, 2020
…rting

Add Battery Voltage Divider Driver
tyalie pushed a commit to tyalie/zmk that referenced this pull request Nov 15, 2022
…rting

Add Battery Voltage Divider Driver
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants