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.

Sign up for GitHub

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

Core encoder/sensor refactor #1039

Merged
merged 10 commits into from
Jun 19, 2023
Merged

Core encoder/sensor refactor #1039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
70e79ad
refactor(sensors): Sensor event channel data, resolution tweaks.
petejohanson Sep 1, 2021
0d23337
refactor(sensors): ec11 rotation sensor value in degrees.
petejohanson Dec 2, 2021
0529949
refactor(config): Select SENSOR as needed.
petejohanson May 26, 2022
d8ccde6
refactor(bluetooth): Bump HoG stack size.
petejohanson May 22, 2023
2c457f5
refactor(shields): Updated ZMK Uno encoder config.
petejohanson Apr 16, 2023
6296925
refactor(bluetooth): Add battery reporting config.
petejohanson Apr 18, 2023
0a386e2
refactor(sensors): Split data handling from triggers.
petejohanson May 4, 2023
2465e3d
refactor(sensors): Use "sensor index" consistently
petejohanson May 17, 2023
ecabd88
fix(sensors): Clean ups based on code review.
petejohanson Jun 18, 2023
8cfbd64
feat(blog): Add post about sensor refactor.
petejohanson May 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ endif()
target_sources_ifdef(CONFIG_ZMK_RGB_UNDERGLOW app PRIVATE src/behaviors/behavior_rgb_underglow.c)
target_sources_ifdef(CONFIG_ZMK_BACKLIGHT app PRIVATE src/behaviors/behavior_backlight.c)

target_sources_ifdef(CONFIG_ZMK_BLE app PRIVATE src/events/battery_state_changed.c)
target_sources_ifdef(CONFIG_ZMK_BLE app PRIVATE src/battery.c)
target_sources_ifdef(CONFIG_ZMK_BATTERY_REPORTING app PRIVATE src/events/battery_state_changed.c)
target_sources_ifdef(CONFIG_ZMK_BATTERY_REPORTING app PRIVATE src/battery.c)

target_sources_ifdef(CONFIG_ZMK_SPLIT app PRIVATE src/events/split_peripheral_status_changed.c)
add_subdirectory(src/split)
Expand Down
28 changes: 25 additions & 3 deletions app/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,9 +114,9 @@ menuconfig ZMK_BLE
select BT_SMP_APP_PAIRING_ACCEPT
select BT_PERIPHERAL
select BT_DIS
select BT_BAS
select BT_SETTINGS
select SETTINGS
imply ZMK_BATTERY_REPORTING

if ZMK_BLE

Expand All @@ -131,7 +131,7 @@ config SYSTEM_WORKQUEUE_STACK_SIZE

config ZMK_BLE_THREAD_STACK_SIZE
int "BLE notify thread stack size"
default 512
default 768

config ZMK_BLE_THREAD_PRIORITY
int "BLE notify thread priority"
Expand Down Expand Up @@ -306,6 +306,12 @@ endmenu

menu "Power Management"

config ZMK_BATTERY_REPORTING
petejohanson marked this conversation as resolved.
Show resolved Hide resolved
bool "Battery level detection/reporting"
default n
select SENSOR
select BT_BAS if ZMK_BLE

config ZMK_IDLE_TIMEOUT
int "Milliseconds of inactivity before entering idle state (OLED shutoff, etc)"
default 30000
Expand Down Expand Up @@ -516,8 +522,24 @@ config ZMK_WPM
bool "Calculate WPM"
default n

config SENSOR
config ZMK_KEYMAP_SENSORS
bool "Enable Keymap Sensors support"
default y
depends on DT_HAS_ZMK_KEYMAP_SENSORS_ENABLED
select SENSOR

if ZMK_KEYMAP_SENSORS

config ZMK_KEYMAP_SENSORS_DEFAULT_TRIGGERS_PER_ROTATION
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between this and ZMK_ENCODERS_DEFAULT_TRIGGERS_PER_ROTATION? Do we need two different configs (with different defaults) that seem to be for the same thing?

int "Default triggers per rotation"
help
Unless overridden for a sensor in the board/shield/devicetree, this value
determines how many times to trigger the bound behavior per full rotation.
For tactile encoders with detents, this usually should match the number of
detents per rotation of the encoder.
default 20

endif # ZMK_KEYMAP_SENSORS

choice CBPRINTF_IMPLEMENTATION
default CBPRINTF_NANO
Expand Down
24 changes: 24 additions & 0 deletions app/boards/shields/zmk_uno/boards/nrf52840dk_nrf52840.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@

/ {
// First, delete the existing basic GPIO based instance.
/delete-node/ encoder;
};

&pinctrl {
qdec_default: qdec_default {
group1 {
psels = <NRF_PSEL(QDEC_A, 1, 11)>,
<NRF_PSEL(QDEC_B, 1, 10)>;
bias-pull-up;
};
};
};

// Set up the QDEC hardware based driver and give it the same label as the deleted node.
encoder: &qdec0 {
status = "okay";
led-pre = <0>;
steps = <80>;
pinctrl-0 = <&qdec_default>;
pinctrl-names = "default";
};
49 changes: 26 additions & 23 deletions app/boards/shields/zmk_uno/zmk_uno.overlay
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@

// Commented out until we add more powerful power domain support
// external_power {
// compatible = "zmk,ext-power-generic";
// label = "EXT_POWER";
// init-delay-ms = <200>;
// control-gpios = <&arduino_header 1 GPIO_ACTIVE_LOW>;
// compatible = "zmk,ext-power-generic";
// label = "EXT_POWER";
// init-delay-ms = <200>;
// control-gpios = <&arduino_header 1 GPIO_ACTIVE_LOW>;
// };

rgb_power {
Expand Down Expand Up @@ -128,14 +128,14 @@
diode-direction = "col2row";

col-gpios
= <&arduino_header 10 GPIO_ACTIVE_HIGH>
, <&arduino_header 9 GPIO_ACTIVE_HIGH>
;
= <&arduino_header 10 GPIO_ACTIVE_HIGH>
, <&arduino_header 9 GPIO_ACTIVE_HIGH>
;

row-gpios
= <&arduino_header 13 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>
, <&arduino_header 11 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>
;
= <&arduino_header 13 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>
, <&arduino_header 11 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>
;

};

Expand All @@ -144,11 +144,11 @@
status = "disabled";

input-gpios
= <&arduino_header 10 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 9 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 11 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
;
= <&arduino_header 10 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 9 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 11 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
;

};

Expand All @@ -157,23 +157,26 @@
toggle-mode;

input-gpios
= <&arduino_header 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 3 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
;
= <&arduino_header 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 3 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
, <&arduino_header 2 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>
;
};

encoder: encoder {
label = "ENCODER";
resolution = <4>;
steps = <80>;
compatible = "alps,ec11";
a-gpios = <&arduino_header 14 GPIO_PULL_UP>;
b-gpios = <&arduino_header 15 GPIO_PULL_UP>;
a-gpios = <&arduino_header 15 GPIO_PULL_UP>;
b-gpios = <&arduino_header 14 GPIO_PULL_UP>;
};

sensors {
compatible = "zmk,keymap-sensors";
sensors = <&encoder>;
triggers-per-rotation = <20>;
left {
triggers-per-rotation = <20>;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand what this is doing. Why do we need to set the triggers per rotation both on the sensors node and on a "left" node which I don't see referenced anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The config here allows setting a global value, or override the setting per sensor. Needed in case, say, you have a tactile encoder with a certain number of detents per rotation, and next to it a linear encoder that you want to trigger behaviors more frequently. This gets pulled in by the sensor code and leveraged by the keymap.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm just confused by the overriding of a "left" encoder on a thing that only has one encoder as far as I can tell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example and testing, more than requirement. Made it easy to comment out portions locally when testing this.

};

};
2 changes: 2 additions & 0 deletions app/drivers/sensor/ec11/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

menuconfig EC11
bool "EC11 Incremental Encoder Sensor"
default y
depends on DT_HAS_ALPS_EC11_ENABLED
depends on GPIO
help
Enable driver for EC11 incremental encoder sensors.
Expand Down
33 changes: 27 additions & 6 deletions app/drivers/sensor/ec11/ec11.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

#include "ec11.h"

#define FULL_ROTATION 360

LOG_MODULE_REGISTER(EC11, CONFIG_SENSOR_LOG_LEVEL);

static int ec11_get_ab_state(const struct device *dev) {
Expand Down Expand Up @@ -59,23 +61,41 @@ static int ec11_sample_fetch(const struct device *dev, enum sensor_channel chan)
drv_data->pulses += delta;
drv_data->ab_state = val;

drv_data->ticks = drv_data->pulses / drv_cfg->resolution;
drv_data->delta = delta;
drv_data->pulses %= drv_cfg->resolution;
// TODO: Temporary code for backwards compatibility to support
// the sensor channel rotation reporting *ticks* instead of delta of degrees.
// REMOVE ME
if (drv_cfg->steps == 0) {
drv_data->ticks = drv_data->pulses / drv_cfg->resolution;
drv_data->delta = delta;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm reading this correctly, settings steps = 0 puts the driver into a mode where it works like it did previously and doesn't provide a position in degrees? Won't that confuse any code that's reading the sensor and expecting degrees?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely would. The expectation is you "enable" both the new steps at the same time you enable the new config in the ZMK sensors node so both sets of "new code" are turned on at the same time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a comment explaining that this is temporary code for backwards compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

drv_data->pulses %= drv_cfg->resolution;
}

return 0;
}

static int ec11_channel_get(const struct device *dev, enum sensor_channel chan,
struct sensor_value *val) {
struct ec11_data *drv_data = dev->data;
const struct ec11_config *drv_cfg = dev->config;
int32_t pulses = drv_data->pulses;

if (chan != SENSOR_CHAN_ROTATION) {
return -ENOTSUP;
}

val->val1 = drv_data->ticks;
val->val2 = drv_data->delta;
drv_data->pulses = 0;

if (drv_cfg->steps > 0) {
val->val1 = (pulses * FULL_ROTATION) / drv_cfg->steps;
val->val2 = (pulses * FULL_ROTATION) % drv_cfg->steps;
if (val->val2 != 0) {
val->val2 *= 1000000;
val->val2 /= drv_cfg->steps;
}
} else {
val->val1 = drv_data->ticks;
val->val2 = drv_data->delta;
}

return 0;
}
Expand Down Expand Up @@ -132,7 +152,8 @@ int ec11_init(const struct device *dev) {
const struct ec11_config ec11_cfg_##n = { \
.a = GPIO_DT_SPEC_INST_GET(n, a_gpios), \
.b = GPIO_DT_SPEC_INST_GET(n, b_gpios), \
COND_CODE_0(DT_INST_NODE_HAS_PROP(n, resolution), (1), (DT_INST_PROP(n, resolution))), \
.resolution = DT_INST_PROP_OR(n, resolution, 1), \
.steps = DT_INST_PROP_OR(n, steps, 0), \
}; \
DEVICE_DT_INST_DEFINE(n, ec11_init, NULL, &ec11_data_##n, &ec11_cfg_##n, POST_KERNEL, \
CONFIG_SENSOR_INIT_PRIORITY, &ec11_driver_api);
Expand Down
1 change: 1 addition & 0 deletions app/drivers/sensor/ec11/ec11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ struct ec11_config {
const struct gpio_dt_spec a;
const struct gpio_dt_spec b;

const uint16_t steps;
const uint8_t resolution;
};

Expand Down
5 changes: 5 additions & 0 deletions app/drivers/zephyr/dts/bindings/sensor/alps,ec11.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,9 @@ properties:
resolution:
type: int
description: Number of pulses per tick
deprecated: true
required: false
petejohanson marked this conversation as resolved.
Show resolved Hide resolved
steps:
petejohanson marked this conversation as resolved.
Show resolved Hide resolved
type: int
description: Number of pulses in one full rotation
required: false
12 changes: 11 additions & 1 deletion app/dts/bindings/zmk,keymap-sensors.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,14 @@ compatible: "zmk,keymap-sensors"
properties:
sensors:
type: phandles
required: true
required: false
triggers-per-rotation:
type: int
required: false

child-binding:
description: Per-sensor configuration settings
properties:
triggers-per-rotation:
type: int
required: false
Loading