Add haptic feedback interface #76343

rriveramcrus · 2024-07-26T16:00:58Z

Adds a haptic driver API and a DRV2605 haptic device driver.

bjarki-andreasen · 2024-07-27T10:19:07Z

samples/drivers/drv2605/src/main.c

Since the configuration of the haptic device is vendor/device specific, it is more efficient to move the/all configuration functions to the vendor/device specific header :)

Specifically, the API haptics_source_config(dev, HAPTICS_SOURCE_ROM, &rom_data); is not portable, whereas haptics_start_output(dev); is.

Forcing the config struct, in this case struct drv2605_rom_data rom_data = {}; to be passed by void reference through a "generic" API is less safe than creating a specific API for the drv2605 which takes a type safe combination of arguments, like:

int haptics_drv2605_source_config_rom(const struct device *dev, const struct drv2605_rom_data *data);

Which is completely type safe (except for the *dev pointer but that's a Zephyr thing).

In summary, device specific configs/features which are not generic enough to be abstractly used by the application (meaning one can switch from one haptic device of one vendor to another, without changing the application), write device specific typesafe APIs for it. Only expose the generic APIs in the haptics API

ack, I see your point here.

bjarki-andreasen · 2024-07-30T07:43:29Z

include/zephyr/drivers/haptics.h

This struct should be moved to the drv2605 header :)

You may also use actual Doxygen comment tags :)

bjarki-andreasen · 2024-07-30T07:46:09Z

dts/bindings/haptics/ti,drv2605.yaml

We typically only use dashes in dts properties, and exclude the vendor prefix, so ti,actuator-mode would be actuator-mode

bjarki-andreasen · 2024-07-30T07:51:35Z

dts/bindings/haptics/ti,drv2605.yaml

Typically string enums are formatted like macros, with no spaces, and shortened as much as possible:

- "1X" - "2X" ... - "DISABLED"

This makes it easy to directly use the value within the drivers by concatenation like:

#define TI_FEEDBACK_BRAKE_FACTOR_1X 1 #define TI_FEEDBACK_BRAKE_FACTOR_2X 2 ... uint8_t break_factor = _CONCAT(TI_FEEDBACK_BRAKE_FACTOR_, DT_INST_STRING_TOKEN(inst, ti_feedback_brake_factor))

bjarki-andreasen · 2024-07-30T07:53:56Z

drivers/haptics/Kconfig.drv2605

Just use the HAPTICS_INIT_PRIORITY in the device driver :) Additionally, to add driver specific configs, hide them behind

if HAPTICS_DRV2605 ... endif

rather than depends on HAPTICS_DRV2605

ack, I'll just use HAPTICS_INIT_PRIORITY :)

bjarki-andreasen · 2024-07-30T07:55:19Z

include/zephyr/drivers/haptics/drv2605.h

The source struct should be moved here instead of being defined in the haptic.h header

bjarki-andreasen · 2024-07-30T07:58:05Z

samples/drivers/drv2605/prj.conf

The device driver ~~should select~~ already selects the driver automatically using depends on DT_HAS_TI_DRV2605_ENABLED and default y making this selection redundant :)

bjarki-andreasen

I think this looks good :)

kartben · 2024-07-29T16:09:29Z

samples/drivers/drv2605/CMakeLists.txt

the sample should live in samples/drivers/haptics/drv2605
Also please add some kind of README :)

ack and ack

kartben · 2024-08-02T15:50:44Z

include/zephyr/drivers/haptics.h

You may also use actual Doxygen comment tags :)

samples/drivers/drv2605/src/main.c

kartben

just spotted minor doxygen issue, lgtm otherwise - thanks!

kartben · 2024-08-05T19:54:00Z

include/zephyr/drivers/haptics/drv2605.h

It's important to keep the first arg to retval a single token (see ex https://builds.zephyrproject.io/zephyr/pr/76343/docs/doxygen/html/group__haptics__interface.html#ga96e0b0fad35d6535adad7536a9411f44)

Suggested change

* @retval < 0 if failed

* @retval <0 if failed

kartben · 2024-08-05T19:54:26Z

include/zephyr/drivers/haptics.h

kartben · 2024-08-05T19:54:40Z

include/zephyr/drivers/haptics.h

Suggested change

* @retval < 0 if failed

* @retval <0 if failed

Introduces a haptics API for use with LRA driver ICs to create haptic feedback events. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

Adds the devicetree bindings for the DRV2605 haptic driver IC. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

Adds support for the DRV2605 haptic driver. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

Adds Haptics build test Signed-off-by: Ricardo Rivera-Matos <[email protected]>

Adds a sample application for the DRV2605 haptic driver IC. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

fabiobaltieri

Cool!

fabiobaltieri

Can you add a maintainers file entry as well?

carlescufi · 2024-08-08T13:38:52Z

@bjarki-andreasen can you take another look please?

rriveramcrus · 2024-08-08T14:00:33Z

@fabiobaltieri,

Can you add a maintainers file entry as well?

Will do in a subsequent PR later today.

kartben · 2024-08-12T10:40:11Z

drivers/haptics/drv2605.c

+		.en_gpio = GPIO_DT_SPEC_INST_GET_OR(inst, ti_en_gpios, {}),                        \
+		.in_trig_gpio = GPIO_DT_SPEC_INST_GET_OR(inst, ti_in_trig_gpios, {}),              \
+		.feedback_brake_factor = DT_INST_ENUM_IDX_OR(inst, ti_feedback_brake_factor, 3),   \
+		.loop_gain = DT_INST_ENUM_IDX_OR(inst, ti_loop_gain, 2),                           \
+		.actuator_mode = DT_INST_ENUM_IDX_OR(inst, ti_actuator_mode, 0),                   \


None of these properties exist in the binding, either due to bad names (start with ti, ... here but not in the yaml file), or not even defined at all in the yaml file (ex. en-gpios), so there is a lot of dead code and potentially a very stuck drv2605 when the EN input pin actually needs to be driven high to even be active :)

Note that i don't think we need/want ti, prefix in the first place

kartben · 2024-11-14T12:36:49Z

@rriveramcrus any chance you could assemble a couple bullet points to add haptics to the release notes?

rriveramcrus · 2024-11-14T17:13:43Z

@rriveramcrus any chance you could assemble a couple bullet points to add haptics to the release notes?

@kartben , Yes I can. (#81409)

rriveramcrus force-pushed the haptic_init branch 4 times, most recently from 5852a26 to e90af08 Compare July 26, 2024 17:43

bjarki-andreasen reviewed Jul 27, 2024

View reviewed changes

rriveramcrus force-pushed the haptic_init branch from e90af08 to 057f4d5 Compare July 29, 2024 18:42

rriveramcrus requested a review from bjarki-andreasen July 29, 2024 21:24

bjarki-andreasen reviewed Jul 30, 2024

View reviewed changes

rriveramcrus force-pushed the haptic_init branch from 057f4d5 to 3eef55c Compare August 2, 2024 19:53

rriveramcrus requested a review from bjarki-andreasen August 5, 2024 13:17

bjarki-andreasen previously approved these changes Aug 5, 2024

View reviewed changes

rriveramcrus marked this pull request as ready for review August 5, 2024 15:29

zephyrbot added area: Samples Samples platform: TI SimpleLink Texas Instruments SimpleLink MCU labels Aug 5, 2024

zephyrbot requested review from kartben, nashif, vaishnavachath and vanti August 5, 2024 15:30

zephyrbot assigned kartben Aug 5, 2024

kartben requested changes Aug 5, 2024

View reviewed changes

rriveramcrus dismissed bjarki-andreasen’s stale review via 994b21e August 5, 2024 19:51

rriveramcrus force-pushed the haptic_init branch 2 times, most recently from 994b21e to 5139e90 Compare August 6, 2024 00:11

rriveramcrus requested a review from kartben August 6, 2024 12:30

kartben requested changes Aug 6, 2024

View reviewed changes

rriveramcrus and others added 5 commits August 6, 2024 21:18

haptics: Introduces a haptics API

c34efeb

Introduces a haptics API for use with LRA driver ICs to create haptic feedback events. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

dts: haptics: Adds the DRV2605 devicetree bindings

ec10ce6

Adds the devicetree bindings for the DRV2605 haptic driver IC. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

haptics: Introduces support for DRV2605 Haptic Driver

82c7c14

Adds support for the DRV2605 haptic driver. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

tests: build_all: Adds Haptics build test

aed5e44

Adds Haptics build test Signed-off-by: Ricardo Rivera-Matos <[email protected]>

samples: drv2605: Adds a DRV2605 sample application

70d1392

Adds a sample application for the DRV2605 haptic driver IC. Signed-off-by: Ricardo Rivera-Matos <[email protected]>

rriveramcrus force-pushed the haptic_init branch from 5139e90 to 70d1392 Compare August 6, 2024 21:19

kartben approved these changes Aug 6, 2024

View reviewed changes

rriveramcrus requested a review from kartben August 7, 2024 11:48

kartben requested a review from fabiobaltieri August 7, 2024 11:50

rriveramcrus requested a review from bjarki-andreasen August 7, 2024 14:42

fabiobaltieri approved these changes Aug 8, 2024

View reviewed changes

fabiobaltieri reviewed Aug 8, 2024

View reviewed changes

bjarki-andreasen approved these changes Aug 8, 2024

View reviewed changes

carlescufi merged commit 0c7e877 into zephyrproject-rtos:main Aug 8, 2024

kartben reviewed Aug 12, 2024

View reviewed changes

rriveramcrus mentioned this pull request Aug 12, 2024

DRV2605 DT Property Cleanup #76969

Merged

Add haptic feedback interface #76343

Add haptic feedback interface #76343

Uh oh!

Conversation

rriveramcrus commented Jul 26, 2024

Uh oh!

bjarki-andreasen Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarki-andreasen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kartben left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabiobaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

fabiobaltieri left a comment

Choose a reason for hiding this comment

Uh oh!

carlescufi commented Aug 8, 2024

Uh oh!

rriveramcrus commented Aug 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartben commented Nov 14, 2024

Uh oh!

rriveramcrus commented Nov 14, 2024

Uh oh!

Reviewers

Assignees

Labels

bjarki-andreasen Jul 27, 2024 •

edited

Loading

bjarki-andreasen Jul 30, 2024 •

edited

Loading