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

Use steno mode from base #5220

Merged
merged 1 commit into from Feb 28, 2019

Conversation

Projects
None yet
2 participants
@tschulte
Copy link
Contributor

commented Feb 22, 2019

Description

AFAIK this was the first layout to use a steno mode, and I think the keymap_steno was derived from it. But it was never changed to use keymap_steno.h.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

  • None

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).

Currently these changes break the layout due to #4578. When changing to Gemini mode, it works.

Since this might be used as a copy template, I think it is better to have it changed to use keymap_steno.h.

You might consider waiting until #4578 is fixed before accepting this pull request, though.

// Runs just one time when the keyboard initializes.
void matrix_init_user(void) {
steno_set_mode(STENO_MODE_BOLT); // or STENO_MODE_GEMINI

This comment has been minimized.

Copy link
@drashna

drashna Feb 24, 2019

Member

Once this is set, you shouldn't need to set it again.
This function writes to EEPROM, so it's saved.

But this should cause no harm.

But it may be better to add this to eeconfig_init_user, so that if the eeprom is reset, it's defaulted back to this value.

This comment has been minimized.

Copy link
@tschulte

tschulte Feb 25, 2019

Author Contributor

OK, I didn't know that. I just discovered it does not work at all, when not called.

Should I then also update the documentation in

```C
void matrix_init_user() {
steno_set_mode(STENO_MODE_GEMINI); // or STENO_MODE_BOLT
}
```
?

Or is it possible to add a check to

void steno_init() {
if (!eeconfig_is_enabled()) {
eeconfig_init();
}
mode = eeprom_read_byte(EECONFIG_STENOMODE);
}
to default to BOLT. I'm no C developer, therefore I don't know if

  mode = eeprom_read_byte(EECONFIG_STENOMODE);
  if (!mode) {
    mode = STENO_MODE_BOLT;
  }

would work.

The documentation already says so: "By default QMK will speak the TX Bolt protocol but can be switched to GeminiPR; the last protocol used is stored in non-volatile memory so QMK will use the same protocol on restart."

This comment has been minimized.

Copy link
@tschulte

tschulte Feb 25, 2019

Author Contributor

OK, I think it can't be if (!mode), but if (mode != STENO_MODE_BOLT && mode != STENO_MODE_GEMINI).

Another problem I had was that even without the change in process_steno.c, I could now even remove my matrix_init_user completely, and it still worked, because the value is now stored in the eeprom.

So I had to reset the value first. But then the eeconfig_init_user method is not called at all. Will this method only be called when the eeprom has never been initialized? So it can't be used to configure it to GEMINI, when it was previously configured to BOLT? Therefore having the code in matrix_init_user means "do it regardless of what I said before, everytime the keyboard is powered up", whereas having it in eeconfig_init_user means "do this if I have never ever anytime anywhen initialized the eeprom"?

In that case having the code in matrix_init_user seems better, unless there is a key defined to switch protocol. All other layouts that use steno also define matrix_init_user instead of _eeconfig_init_user. And we must change process_steno.c to fallback to BOLT if there is no valid value in the eeprom yet.

This comment has been minimized.

Copy link
@drashna

drashna Feb 26, 2019

Member

Well, the enum has it listed as 0 for Bolt.

So that would be the default value, actually. So you wouldn't need to call anything here, unless you wanted to use Gemini.

So really, the entire function could be removed here.

typedef enum { STENO_MODE_BOLT, STENO_MODE_GEMINI } steno_mode_t;

This comment has been minimized.

Copy link
@tschulte

tschulte Feb 27, 2019

Author Contributor

That's odd, because at first I did not have that line at all, and it was not working. Maybe somehow there was something at that position in the eeprom. Maybe because I started with 2 different firmwares before switching to qmk, there was already something completely different at that position.

I will create a pull request for process_steno.c to check for a valid mode and if invalid default to bolt mode.

And for ergodox_ez/steno (and my own layout, which I may later provide for the layouts/community/ergodox folder) I would either opt to leave the matrix_init_user method, because

  1. It's the save way to have the mode at the correct value, even if for some reason the value at the eeprom is at e.g. gemini mode
  2. Most of the other layouts that use steno mode also do it this way
  3. The documentation states exactly that

Or alternatively the layout introduces a switch to change the mode.

But this is not the layout I am using, just the layout I wanted to use as source to copy, so I just wanted to have it in good shape before doing that. I am fine with both alternatives, but since I am not the original author of the layout I wanted it to work exactly the same as before. Maybe @dragon788 as the original author of this layout has something to say about this.

This comment has been minimized.

Copy link
@drashna

drashna Feb 28, 2019

Member

It's possible that you ad something else there, but that would be odd, too.

Either way, I think this should be okay, as is.

@drashna drashna added the keymap label Feb 24, 2019

@drashna

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Thanks!

@drashna drashna merged commit 7470317 into qmk:master Feb 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tschulte tschulte deleted the tschulte:ergodox_neo_refactoring branch Mar 1, 2019

HokieGeek added a commit to HokieGeek/qmk_firmware that referenced this pull request Mar 5, 2019

slugger7 added a commit to slugger7/qmk_firmware that referenced this pull request Mar 7, 2019

ChrissiQ added a commit to ChrissiQ/qmk_firmware that referenced this pull request Mar 15, 2019

sriehl added a commit to sriehl/qmk_firmware that referenced this pull request Mar 15, 2019

zer09 added a commit to zer09/qmk_firmware that referenced this pull request Mar 18, 2019

chie4hao added a commit to chie4hao/qmk_firmware that referenced this pull request Mar 28, 2019

slugger7 added a commit to slugger7/qmk_firmware that referenced this pull request Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.