Haptic and solenoid cleanup #9700

purdeaandrei · 2020-07-11T02:19:23Z

Description

Overall this PR fixes:

how haptic configuration is first set up if the eeprom is empty/corrupt, or if previous firmware didn't have haptic enabled.
a bunch of small bugs related to solenoid (reset, loading of settings from eeprom, etc...)
makes solenoid a little more configurable

For more detailed descriptions please see my individual commits for descriptions of my changes.

Types of Changes

Issues Fixed or Closed by This PR

N/A (i think)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

zvecr

Theres a slight behaviour difference between haptic_dwell_increase and haptic_dwell_decrease, so im adding the "breaking changes" label till we know if its going to have to go via develop.

Also the build is failing.

purdeaandrei · 2020-07-17T18:32:55Z

@zvecr I pushed a fix for the build issue.
I would like to argue that the behavior change is a bugfix, and it never should have allowed the dwell time to overflow, and get a less-then-MIN value.
Before this dwell time wasn't really saved in eeprom while it was clear from code that that was the intent. Would the fixing of that also make this PR a breaking change?

purdeaandrei · 2020-07-17T18:36:51Z

Also, there are no keyboards within the QMK tree, that use the core solenoid functionality.
(There's some mtdjr user code for solenoid but that is different, looks to be the originator of QMK solenoid code)
The functionality of saving/restoring to eeprom, and resetting the solenoid settings was so broken, that I find it hard
to believe anyone's using it out of tree in its current form.

EDIT: not true. I probably made a mistake while initially looking for keyboards/keymaps using the solenoid code.

zvecr · 2020-07-17T19:02:11Z

We care about out of tree breakages too, some of the docs might need a little update here and there but it's mostly covered by https://docs.qmk.fm/#/breaking_changes_instructions. The removal of an api, which while i agree with it's removal, is enough in most cases that it wouldn't be within a patch release.

I will leave it for other collabs to express their stance. However i would add that an outside view of issue classification and chosen workflow might not align with the direction of the project.

docs/feature_haptic_feedback.md

drashna · 2020-07-30T23:27:37Z

I'm not sure what the buzz part is supposed to do. I don't really notice a change in behavior.

purdeaandrei · 2020-08-14T06:41:31Z

@drashna

I probably made a mistake while initially looking for keyboards/keymaps using the solenoid code, so what I said above about not being any keyboards using it is wrong, there are a few using it in the QMK tree.

I'm not sure what the buzz part is supposed to do. I don't really notice a change in behavior.

I did not write this code initially, but based on reading the code, I believe it means that in buzz mode the solenoid is tapped multiple times for each key press.

In mtdjr's non-core code in users/mtdjr/solenoid.h SOLENOID_MIN_DWELL was only used to specify half the period for the buzzing solenoid signal.
So the macro meanings were:

SOLENOID_DEFAULT_DWELL - the starting dwell time setting. (in the buzz case this specifies the length of the entire waveform)
SOLENOID_MAX_DWELL - this specified the maximum dwell time, beyond which the solenoid dwell time was not allowed to be incremented when adjusting it
SOLENOID_MIN_DWELL - this is NOT the opposite of SOLENOID_MAX_DWELL in the (what appears to be) original mtdjr code.

I think someone took the mtdjr code and driverized it (imported into core), and during driverization, some things changed:

using eeprom to store settings (incorrectly implemented)
SOLENOID_MIN_DWELL still means the same as it used to mean in the original code, but now it also means a lower limit when adjusting the solenoid dwell time. In the original code the lower limit was just zero.
the adjustment code was changed to go round robin, although incorrectly.
(not complete list)

So to see buzz do anything special, for example you need to:
0) Use the code on this branch. There's lots of bugs otherwise.

Have buzz enabled by using the HPT_BUZ key
increase solenoid dwell time to maybe 8 times the SOLENOID_MIN_DWELL_TIME setting. This will make the solenoid tap 4 times for each keypress. I don't know if you will hear the difference, but I tested this with a logic analyzer and it works like this.

Again, without this PR there are plenty of bugs. For instance the reset doesn't reset. The saved settings (such as solenoid dwell time) aren't properly reloaded on power cycle. I noted that by default solenoid buzz was on,.so you won't actually see anything increasing dwell time, except if you increase it beyond 2 times the minimum. Please see my individual commit messages for more detailed descriptions of the bugs that I fixed.

Andrei

purdeaandrei · 2020-08-29T13:45:59Z

@drashna Alright, so since this change is still categorized as a breaking change, it will definitely have to be retargeted to develop. So I'm gonna do that now...

purdeaandrei · 2020-08-29T13:57:26Z

Alright, I rebased it to develop, please let me know if there are any more pending actions on me for this PR.

purdeaandrei · 2020-09-06T20:23:31Z

@noroadsleft What happened here? The force-push screwed up my PRs history...
Is that force-push correct, or a mistake?
Do I need to rebase again?

purdeaandrei · 2020-09-06T20:37:53Z

@noroadsleft I see commit 0f9b7b9 was inserted underneath an existing stack of commits that were already on develop, so that's why the force push was done. Any reason why that commit itself could not have been rebased, to not edit history?

drashna · 2020-09-07T03:55:17Z

IIRC, a "simple" way to fix this is with:

git reset upstream/develop --hard
git cherrypick d2857d3^...739cd61
git push --force-with-lease

(the "with lease" part isn't strictly necessary, it's just good habit, IMO)

You can test this with another branch, and see if that cleans up things, first. And then do to this branch.

noroadsleft · 2020-09-12T11:43:11Z

I see commit 0f9b7b9 was inserted underneath an existing stack of commits that were already on develop, so that's why the force push was done. Any reason why that commit itself could not have been rebased, to not edit history?

QMK's workflow for the develop branch is that pull requests that target the branch are merged the same way PRs to master are, and then the develop branch is rebased upon master once a week. Unfortunately the result is that long-standing PRs to develop also need to be rebased regularly.

You can follow the instructions drashna gave above in situations where you need to rebase because the target branch (develop) has changed.

We usually try to rebase develop every Friday at 12:00 US Pacific Time (UTC -7:00 at the moment).

Speaking of which, it needs to be rebased again...

These functions modify the argument, and so they do nothing. Note: versions of these functions exist in mtdjr's user folder, however to core solenoid support and mtdjr user-specific solenoid support are exclusive (only one can be used at a time). So removing these confusing functions does no harm.

The previous code allowed dwell time to go 1ms below the configured minimum. This change corrects it.

…ck to minimum, not to 1 The previous code used to jump back to 1, which might be way under the configured minimum setting.

This is because the dwell time is stored in two variables.

… dwell time. This is needed because dwell time is configured in two variables.

…lenoid isn't

purdeaandrei · 2020-09-13T05:06:55Z

Rebased

noroadsleft

Slight __attribute__ ((weak)) ✔️ because I don't have any experience with the Haptic or Solenoid features, but the code delta looks good to me.

purdeaandrei · 2020-09-28T00:19:59Z

Is there anything I can do to help this progress forward?

purdeaandrei · 2020-10-09T09:39:50Z

Hello,

Would it help if I asked some of my firmware's users who use a solenoid to comment here about my solenoid fixes?

I understand they are not official approvers, but maybe it will increase approver confidence in the changeset?

Or is there anything else I can do to make this move forward? It's been almost 3 months since the last commit (there were only rebases after that, the last rebase almost a month ago).

Andrei

drashna · 2020-10-11T00:54:45Z

I think it should be okay (I have tested on a couple of my boards, and it seems to be fine).

noroadsleft · 2020-10-13T22:06:50Z

@drashna I'd be okay with merging this if it has your approval. I can't test this at all (don't own any haptic or solenoid devices).

noroadsleft · 2020-10-17T20:31:30Z

Thanks!

* solenoid: remove two functions that do nothing. These functions modify the argument, and so they do nothing. Note: versions of these functions exist in mtdjr's user folder, however to core solenoid support and mtdjr user-specific solenoid support are exclusive (only one can be used at a time). So removing these confusing functions does no harm. * solenoid: bugfix: don't allow dwell time to go 1ms below minimum time. The previous code allowed dwell time to go 1ms below the configured minimum. This change corrects it. * solenoid: bugfix: when incrementing above maximum dwell time, jump back to minimum, not to 1 The previous code used to jump back to 1, which might be way under the configured minimum setting. * solenoid: bugfix: on startup actually use the eeprom-stored dwell-time This is because the dwell time is stored in two variables. * solenoid: bugfix: on haptic_reset, actually use the newly set default dwell time. This is needed because dwell time is configured in two variables. * solenoid: on HPT_RST set buzz to a default value * solenoid: buzz: reworked to make more configurable. Previous behaviour maintained. * solenoid: documentation: clarify meaning of dwell time * solenoid: add feature SOLENOID_DWELL_STEP_SIZE * solenoid: documentation: added note about the precision of the solenoid time settings * haptic: Correctly call haptic_reset when eeprom is corrupt. * haptic: improve what happens if haptic is enabled without erasing eeprom * haptic: improve what happens if solenoid is enabled without erasing eeprom * drivers/haptic: fix compilation issue, when haptic is enabled, but solenoid isn't

* Branch point for 2020 November 28 Breaking Change * Remove matrix_col_t to allow MATRIX_ROWS > 32 (#10183) * Add support for soft serial to ATmega32U2 (#10204) * Change MIDI velocity implementation to allow direct control of velocity value (#9940) * Add ability to build a subset of all keyboards based on platform. * Actually use eeprom_driver_init(). * Make bootloader_jump weak for ChibiOS. (#10417) * Joystick 16-bit support (#10439) * Per-encoder resolutions (#10259) * Share button state from mousekey to pointing_device (#10179) * Add hotfix for chibios keyboards not wake (#10088) * Add advanced/efficient RGB Matrix Indicators (#8564) * Naming change. * Support for STM32 GPIOF,G,H,I,J,K (#10206) * Add milc as a dependency and remove the installed milc (#10563) * ChibiOS upgrade: early init conversions (#10214) * ChibiOS upgrade: configuration file migrator (#9952) * Haptic and solenoid cleanup (#9700) * XD75 cleanup (#10524) * OLED display update interval support (#10388) * Add definition based on currently-selected serial driver. (#10716) * New feature: Retro Tapping per key (#10622) * Allow for modification of output RGB values when using rgblight/rgb_matrix. (#10638) * Add housekeeping task callbacks so that keyboards/keymaps are capable of executing code for each main loop iteration. (#10530) * Rescale both ChibiOS and AVR backlighting. * Reduce Helix keyboard build variation (#8669) * Minor change to behavior allowing display updates to continue between task ticks (#10750) * Some GPIO manipulations in matrix.c change to atomic. (#10491) * qmk cformat (#10767) * [Keyboard] Update the Speedo firmware for v3.0 (#10657) * Maartenwut/Maarten namechange to evyd13/Evy (#10274) * [quantum] combine repeated lines of code (#10837) * Add step sequencer feature (#9703) * aeboards/ext65 refactor (#10820) * Refactor xelus/dawn60 for Rev2 later (#10584) * add DEBUG_MATRIX_SCAN_RATE_ENABLE to common_features.mk (#10824) * [Core] Added `add_oneshot_mods` & `del_oneshot_mods` (#10549) * update chibios os usb for the otg driver (#8893) * Remove HD44780 References, Part 4 (#10735) * [Keyboard] Add Valor FRL TKL (+refactor) (#10512) * Fix cursor position bug in oled_write_raw functions (#10800) * Fixup version.h writing when using SKIP_VERSION=yes (#10972) * Allow for certain code in the codebase assuming length of string. (#10974) * Add AT90USB support for serial.c (#10706) * Auto shift: support repeats and early registration (#9826) * Rename ledmatrix.h to match .c file (#7949) * Split RGB_MATRIX_ENABLE into _ENABLE and _DRIVER (#10231) * Split LED_MATRIX_ENABLE into _ENABLE and _DRIVER (#10840) * Merge point for 2020 Nov 28 Breaking Change

* Branch point for 2020 November 28 Breaking Change * Remove matrix_col_t to allow MATRIX_ROWS > 32 (qmk#10183) * Add support for soft serial to ATmega32U2 (qmk#10204) * Change MIDI velocity implementation to allow direct control of velocity value (qmk#9940) * Add ability to build a subset of all keyboards based on platform. * Actually use eeprom_driver_init(). * Make bootloader_jump weak for ChibiOS. (qmk#10417) * Joystick 16-bit support (qmk#10439) * Per-encoder resolutions (qmk#10259) * Share button state from mousekey to pointing_device (qmk#10179) * Add hotfix for chibios keyboards not wake (qmk#10088) * Add advanced/efficient RGB Matrix Indicators (qmk#8564) * Naming change. * Support for STM32 GPIOF,G,H,I,J,K (qmk#10206) * Add milc as a dependency and remove the installed milc (qmk#10563) * ChibiOS upgrade: early init conversions (qmk#10214) * ChibiOS upgrade: configuration file migrator (qmk#9952) * Haptic and solenoid cleanup (qmk#9700) * XD75 cleanup (qmk#10524) * OLED display update interval support (qmk#10388) * Add definition based on currently-selected serial driver. (qmk#10716) * New feature: Retro Tapping per key (qmk#10622) * Allow for modification of output RGB values when using rgblight/rgb_matrix. (qmk#10638) * Add housekeeping task callbacks so that keyboards/keymaps are capable of executing code for each main loop iteration. (qmk#10530) * Rescale both ChibiOS and AVR backlighting. * Reduce Helix keyboard build variation (qmk#8669) * Minor change to behavior allowing display updates to continue between task ticks (qmk#10750) * Some GPIO manipulations in matrix.c change to atomic. (qmk#10491) * qmk cformat (qmk#10767) * [Keyboard] Update the Speedo firmware for v3.0 (qmk#10657) * Maartenwut/Maarten namechange to evyd13/Evy (qmk#10274) * [quantum] combine repeated lines of code (qmk#10837) * Add step sequencer feature (qmk#9703) * aeboards/ext65 refactor (qmk#10820) * Refactor xelus/dawn60 for Rev2 later (qmk#10584) * add DEBUG_MATRIX_SCAN_RATE_ENABLE to common_features.mk (qmk#10824) * [Core] Added `add_oneshot_mods` & `del_oneshot_mods` (qmk#10549) * update chibios os usb for the otg driver (qmk#8893) * Remove HD44780 References, Part 4 (qmk#10735) * [Keyboard] Add Valor FRL TKL (+refactor) (qmk#10512) * Fix cursor position bug in oled_write_raw functions (qmk#10800) * Fixup version.h writing when using SKIP_VERSION=yes (qmk#10972) * Allow for certain code in the codebase assuming length of string. (qmk#10974) * Add AT90USB support for serial.c (qmk#10706) * Auto shift: support repeats and early registration (qmk#9826) * Rename ledmatrix.h to match .c file (qmk#7949) * Split RGB_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10231) * Split LED_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10840) * Merge point for 2020 Nov 28 Breaking Change

* solenoid: remove two functions that do nothing. These functions modify the argument, and so they do nothing. Note: versions of these functions exist in mtdjr's user folder, however to core solenoid support and mtdjr user-specific solenoid support are exclusive (only one can be used at a time). So removing these confusing functions does no harm. * solenoid: bugfix: don't allow dwell time to go 1ms below minimum time. The previous code allowed dwell time to go 1ms below the configured minimum. This change corrects it. * solenoid: bugfix: when incrementing above maximum dwell time, jump back to minimum, not to 1 The previous code used to jump back to 1, which might be way under the configured minimum setting. * solenoid: bugfix: on startup actually use the eeprom-stored dwell-time This is because the dwell time is stored in two variables. * solenoid: bugfix: on haptic_reset, actually use the newly set default dwell time. This is needed because dwell time is configured in two variables. * solenoid: on HPT_RST set buzz to a default value * solenoid: buzz: reworked to make more configurable. Previous behaviour maintained. * solenoid: documentation: clarify meaning of dwell time * solenoid: add feature SOLENOID_DWELL_STEP_SIZE * solenoid: documentation: added note about the precision of the solenoid time settings * haptic: Correctly call haptic_reset when eeprom is corrupt. * haptic: improve what happens if haptic is enabled without erasing eeprom * haptic: improve what happens if solenoid is enabled without erasing eeprom * drivers/haptic: fix compilation issue, when haptic is enabled, but solenoid isn't

zvecr added the core label Jul 17, 2020

zvecr requested changes Jul 17, 2020

View reviewed changes

zvecr added breaking_change Changes that need to wait for a version increment enhancement labels Jul 17, 2020

zvecr requested a review from a team July 17, 2020 12:24

drashna reviewed Jul 24, 2020

View reviewed changes

docs/feature_haptic_feedback.md Show resolved Hide resolved

drashna approved these changes Aug 22, 2020

View reviewed changes

drashna requested a review from a team August 22, 2020 21:48

purdeaandrei changed the base branch from master to develop August 29, 2020 13:49

purdeaandrei force-pushed the f_haptic_and_solenoid_cleanup branch from c5a7cb3 to 739cd61 Compare August 29, 2020 13:54

noroadsleft force-pushed the develop branch 2 times, most recently from 3a8e1b8 to a1b96ba Compare September 4, 2020 22:03

noroadsleft force-pushed the develop branch from a1b96ba to 3430907 Compare September 12, 2020 11:46

purdeaandrei added 6 commits September 13, 2020 08:03

solenoid: bugfix: don't allow dwell time to go 1ms below minimum time.

9fe10b6

The previous code allowed dwell time to go 1ms below the configured minimum. This change corrects it.

solenoid: bugfix: when incrementing above maximum dwell time, jump ba…

5d4796e

…ck to minimum, not to 1 The previous code used to jump back to 1, which might be way under the configured minimum setting.

solenoid: bugfix: on startup actually use the eeprom-stored dwell-time

f106ce5

This is because the dwell time is stored in two variables.

solenoid: bugfix: on haptic_reset, actually use the newly set default…

6055e03

… dwell time. This is needed because dwell time is configured in two variables.

solenoid: on HPT_RST set buzz to a default value

afeef88

drivers/haptic: fix compilation issue, when haptic is enabled, but so…

d5c748e

…lenoid isn't

purdeaandrei force-pushed the f_haptic_and_solenoid_cleanup branch from 739cd61 to d5c748e Compare September 13, 2020 05:04

noroadsleft approved these changes Sep 14, 2020

View reviewed changes

noroadsleft requested a review from a team September 14, 2020 08:01

noroadsleft merged commit ac2e711 into qmk:develop Oct 17, 2020

Razerban pushed a commit to Razerban/qmk_firmware that referenced this pull request Feb 1, 2021

Haptic and solenoid cleanup qmk#9700

c39eeac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Haptic and solenoid cleanup #9700

Haptic and solenoid cleanup #9700

purdeaandrei commented Jul 11, 2020

zvecr left a comment

purdeaandrei commented Jul 17, 2020

purdeaandrei commented Jul 17, 2020 •

edited

Loading

zvecr commented Jul 17, 2020 •

edited

Loading

drashna commented Jul 30, 2020 •

edited

Loading

purdeaandrei commented Aug 14, 2020 •

edited

Loading

purdeaandrei commented Aug 29, 2020

purdeaandrei commented Aug 29, 2020

purdeaandrei commented Sep 6, 2020

purdeaandrei commented Sep 6, 2020 •

edited

Loading

drashna commented Sep 7, 2020

noroadsleft commented Sep 12, 2020

purdeaandrei commented Sep 13, 2020

noroadsleft left a comment

purdeaandrei commented Sep 28, 2020

purdeaandrei commented Oct 9, 2020

drashna commented Oct 11, 2020

noroadsleft commented Oct 13, 2020

noroadsleft commented Oct 17, 2020

Haptic and solenoid cleanup #9700

Haptic and solenoid cleanup #9700

Conversation

purdeaandrei commented Jul 11, 2020

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

zvecr left a comment

Choose a reason for hiding this comment

purdeaandrei commented Jul 17, 2020

purdeaandrei commented Jul 17, 2020 • edited Loading

zvecr commented Jul 17, 2020 • edited Loading

drashna commented Jul 30, 2020 • edited Loading

purdeaandrei commented Aug 14, 2020 • edited Loading

purdeaandrei commented Aug 29, 2020

purdeaandrei commented Aug 29, 2020

purdeaandrei commented Sep 6, 2020

purdeaandrei commented Sep 6, 2020 • edited Loading

drashna commented Sep 7, 2020

noroadsleft commented Sep 12, 2020

purdeaandrei commented Sep 13, 2020

noroadsleft left a comment

Choose a reason for hiding this comment

purdeaandrei commented Sep 28, 2020

purdeaandrei commented Oct 9, 2020

drashna commented Oct 11, 2020

noroadsleft commented Oct 13, 2020

noroadsleft commented Oct 17, 2020

purdeaandrei commented Jul 17, 2020 •

edited

Loading

zvecr commented Jul 17, 2020 •

edited

Loading

drashna commented Jul 30, 2020 •

edited

Loading

purdeaandrei commented Aug 14, 2020 •

edited

Loading

purdeaandrei commented Sep 6, 2020 •

edited

Loading