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

fix: Disable battery sensors if reporting is off #1867

Merged
merged 2 commits into from
Jul 19, 2023

Conversation

caksoylar
Copy link
Contributor

Currenly when ZMK_BATTERY_REPORTING is turned off, builds with battery sensors fail due to a kernel init priority missing (ex. 1, 2), which I think is due to SENSOR Kconfig not getting selected.

Ideally the sensors wouldn't be enabled at all if battery level reporting is disabled but having them in the DT automatically enables them. This change disables them (in a bit of a roundabout way) through disabling ZMK_BATTERY:

add_subdirectory_ifdef(CONFIG_ZMK_BATTERY battery)

Another option to directly disable the symbols would be to add depends on ZMK_BATTERY_REPORTING on each sensor, but this seemed slightly better to me. Let me know if that makes more sense.

Alternatively, maybe there is a more direct way to fix the kernel init priority warning, in which case the sensors would get compiled but not have an effect if reporting is disabled.

@caksoylar caksoylar added core Core functionality/behavior of ZMK sensors labels Jul 14, 2023
@caksoylar caksoylar requested a review from joelspadin July 14, 2023 18:05
@caksoylar caksoylar requested a review from a team as a code owner July 14, 2023 18:05
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.

One thought on the approach chosen here.

Comment on lines 16 to 17
select ZMK_BATTERY
imply ZMK_BATTERY
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems reasonable, but why not also make this depends on SENSOR as an alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand you correctly that sounds like one of the other alternatives I mentioned:

Another option to directly disable the symbols would be to add depends on ZMK_BATTERY_REPORTING on each sensor, but this seemed slightly better to me. Let me know if that makes more sense.

But depends on SENSOR is probably better that that 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, in that case the battery sensors would still get compiled in if SENSOR is enabled through some other symbol like ZMK_KEYMAP_SENSORS.

Copy link
Contributor Author

@caksoylar caksoylar Jul 15, 2023

Choose a reason for hiding this comment

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

I pushed a fixup commit that converts to that; everything builds OK but enabling sensors gets the battery sensors (but not reporting) enabled as well:

> west build -d build/kl2 -b nice_nano_v2 -- -DSHIELD=kyria_left -DCONFIG_ZMK_BATTERY_REPORTING=n -DCONFIG_EC11=y
[...]
> rg BATTERY build/kl2/zephyr/.config
87:# CONFIG_ZMK_BATTERY_REPORTING is not set
238:CONFIG_DT_HAS_ZMK_BATTERY_NRF_VDDH_ENABLED=y
481:CONFIG_ZMK_BATTERY=y
482:CONFIG_ZMK_BATTERY_NRF_VDDH=y

Not sure if that's an issue or not, I can keep or drop the commit depending on what you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's expected behavior, if the nodes are enabled, and the necessary support Kconfig symbols are enabled, we auto-enable the driver. You'd either disable the node in the DTS, or disable it explicitly in the Kconfig as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, does the current state look good in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I believe this is correct now. Thanks!

@petejohanson petejohanson self-assigned this Jul 17, 2023
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 ee1e135 into zmkfirmware:main Jul 19, 2023
@caksoylar caksoylar deleted the fix-battery-reporting branch July 19, 2023 07:20
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 sensors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants