Sequencer keycodes are not static

[fauxpark]

The recent addition of the step sequencer feature in #9703 has introduced a small roadblock in refactoring the Quantum keycode values to be static (ie. removing all the ifdefs in such a way as to not break VIA). The SQ_S(), SQ_R() and SQ_T() keycodes depend on the values of SEQUENCER_STEPS, SEQUENCER_RESOLUTIONS and SEQUENCER_TRACKS which can be configured by the user. As such, it is not possible to know exactly what value SEQUENCER_STEP_MAX will be, and thus impossible for VIA to support these keycodes. Worse, any new features which add keycodes will be unable to be supported by VIA either, as their values can also not be known.

As it turns out, VIA has already been broken by #9940 inserting a new MIDI velocity keycode - although MIDI is not supported by VIA, some of the keycodes are still counted in the enum even if the feature is disabled, and so everything after it is now offset by one. Maybe we should take this opportunity to at least get this ifdef removal done.

@rbelouin would it be possible to refactor the sequencer feature in some way to fix this?

[ruddy17]

I confirm, after #9940 all RGB control keycodes are shifted in VIA, which leads to wrong keycode indication.

[rbelouin]

I don’t know about VIA, is there a place where I can read more about it?

[fauxpark]

https://caniusevia.com/

There is not much info there on what exactly it is or does, but essentially, it is a feature of QMK, and a host-side app, which allows you to change your keymap without flashing by storing it in EEPROM, and control certain things like RGB and backlighting from the host.

Unfortunately there is no way for the VIA app to know what value each keycode is, because the full enum is not stored in the firmware. It has to maintain its own list of keycode values to send to the keyboard, and hope that it does not change, which it just has. So, every entry in the quantum_keycodes enum must have a defined value that is the same at all times, otherwise when you go to assign certain keycodes in VIA, the mapping will be incorrect.

[fauxpark]

@rbelouin

I had a closer look at what the Sequencer feature uses its keycodes for.

I was hoping to be able to simply replace the min/max enum entries with absolute values (eg. SEQUENCER_TRACK_0 to 7), and change the max to a define depending on what the user has configured (see how MIDI_TONE_MAX is determined).

For the steps, I think this is way too many to be reasonable. Do these absolutely need to be keycodes?

[rbelouin]

I think I need some help to understand what the issue is exactly.

1. How does VIA get broken if the SEQUENCER feature is disabled? Looking at this file, it seems that the enum would be left unchanged if the feature is disabled. Is the goal to make VIA work with a QMK config that is enabling the sequencer?
2. If so, won’t that still break VIA if we define SEQUENCER_TRACK_MIN/SEQUENCER_TRACK_MAX like MIDI_TONE_MIN/MIDI_TONE_MAX? Whenever the user changes MIDI/SEQUENCER configuration, the following keycodes will change, right?

If the number of keycodes potentially assigned to the sequencer is still problematic, maybe we can adapt the feature so that it works with a richer state and combination of keycodes instead. Examples:

• If I press SQ_TRACK_SELECT and KC_1, it selects the first track
• If I press SQ_STEP_EDIT it toggles a mode where pressing KC_Q, KC_W… enables or disables certain steps.

This means we’d only have a few "sequencer mode" keycodes that would not vary with the number of tracks/steps/resolutions the end user wants to configure. But again, I’d like to understand the issue better before jumping on a solution 🙂

[fauxpark]

As I mentioned above, VIA needs to know the exact values of each keycode in order to use them, which means they must always be the same no matter what features are enabled or what defines are set. For instance, take these new keycodes which have been added recently to the develop branch:

Ignoring the other ifdefs above which will shift the values around (I have a PR for that, #12249), the values of these three will be dependent on what the user has set for SEQUENCER_TRACKS and SEQUENCER_STEPS. So VIA will not be able to support them as they are not set in stone. This goes for any new keycodes added in future, as they have to be appended to the end so as to keep the existing values the same.

Wilba, one of the VIA devs, has expressed interest in adding support for all keycodes, even ones that require custom code to do anything useful (eg. Combos and Tap Dance), so it stands to reason the Sequencer would be part of that as well. In that case, the keycodes for this feature must be reorganised so that they do not take into account user preferences. The MIN/MAX defines I would say don't need to be worried about, they are inherently variable and I wouldn't expect VIA to support them.

[wilba]

Would things be easier if all the MIDI and sequencer keycodes are in a different enum that uses a reserved block of integers well away from quantum_keycodes enum and SAFE_RANGE integers where these issues of non-constancy become irrelevant? In other words, quantum_keycodes enum contains keycodes specifically for keyboards and highly useful keyboard features that make it into QMK Core, and something like midi_keycodes enum can have it's first value somewhere well past SAFE_RANGE in a reserved block and continue to be variable depending on build flags.

[fauxpark]

Fragmenting the enum just makes it harder to figure out which ranges are free.

Regarding SAFE_RANGE, IMO it should be moved to where QK_FUNCTION or QK_MACRO currently is, once fn_actions and the old macro system are removed. This way custom keycodes will have a more well defined range of 4096 which will not shrink or change as more core keycodes are added. As it is, there are only ~700ish left between the end of the sequencer keycodes and the next block (MT at 0x6000).

The issue of non-constancy remains relevant if you are still aiming to support all keycodes. The MIDI section is fine now although VIA cannot know how many octaves the user wants - but all the keycode values are stable. I'm also not sure whether MIDI_TONE_KEYCODE_OCTAVES even needs to exist, it appears to be only used in the enum.
Unicode may be an issue for VIA since the ranges for Unicode and Unicodemap overlap, the firmware would have to signal to the app which one it has enabled.

And of course, there is still a big reorganisation of the enum to be done, to reunite things like Magic, Space Cadet and Bluetooth into contiguous blocks. That would probably be the best time to think more carefully about allocation.

[rbelouin]

How’s the MIDI section fine? It seems to me that the value of BL_ON would change depending on how the user configured MIDI_ENABLE_STRICT and MIDI_TONE_KEYCODE_OCTAVES, isn’t that the same issue as for the sequencer keycodes?

[fauxpark]

Take a look at how MIDI_ENABLE_STRICT and MIDI_TONE_KEYCODE_OCTAVES are used in the enum. The former defaults to 0, so the #ifs surrounding pretty much all of the MIDI keycodes evaluate to true. The latter doesn't come into play because they are using ||s, and the left operand has already evaluated to true.

The sole purpose of the two defines was to exclude some of these keycodes from the enum - you would set MIDI_ENABLE_STRICT 1 and then define how many octaves you wanted. This would be a problem except that almost no keymaps (and 0 VIA keymaps!) actually changed them from the default. Thus the values do not change after my PR, and the static asserts in via_ensure_keycode.h agree.

[rbelouin]

Hi, I’m sorry it took so long to reply. I should finally have some time to address the issue.
I’m considering an approach where the user assigns existing keycodes to sequencer actions instead of allocating a dynamic keycode range to this feature; especially since allocating less than 32 "step keycodes" would probably not make sense for a music sequencer. What do you think?

[rbelouin]

Unfortunately there is no way for the VIA app to know what value each keycode is, because the full enum is not stored in the firmware. It has to maintain its own list of keycode values to send to the keyboard, and hope that it does not change, which it just has. So, every entry in the quantum_keycodes enum must have a defined value that is the same at all times, otherwise when you go to assign certain keycodes in VIA, the mapping will be incorrect.

Do you know who’s generally maintaining the list of keycodes on VIA’s side? and what’s their process? Is VIA still broken because of my changes last year? Is this software frequently used by people who compile their own layout or is it more used as an alternative for people who don’t have software-engineering skills?

[drashna]

Sequencer keycodes are now static.

[fauxpark]

Nope