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

[Feature Proposal] #12033

Closed
gsadams opened this issue Feb 27, 2021 · 3 comments
Closed

[Feature Proposal] #12033

gsadams opened this issue Feb 27, 2021 · 3 comments
Labels
enhancement help wanted stale Issues or pull requests that have become inactive without resolution.

Comments

@gsadams
Copy link

gsadams commented Feb 27, 2021

Proposal for improvements to the Split Keyboard Transport protocol

Note: Following an industry-wide push to foster inclusivity, I have replaced the terms "master" and "slave," which are used throughout the QMK split keyboard code, with "conductor" and "follower" in this doc. I'm open to other terminology.

The split transport protocol (background)

The current protocol exchanges buffers of data between the two sides. The exact details differ a bit between I2C and serial transport, and the bits-on-the-wire format differs a little between the bit-banging and USART serial methods.

But the basic idea is the same: Transfer some data from conductor to follower and some data from follower to conductor periodically. In the serial transports, there is an identifier of which set of buffers is going to be transported (the RGB light config or the martix-and-everything-else), while in I2C, each transfer writes to or reads from each of several "register" addresses on the follower side, where a register can be as large as the entire half-matrix buffer.

(We'll ignore the non-SERIAL_USE_MULTI_TRANSACTION case for now.)

Shortcomings of the current protocol

There are two main classes of limitations with this protocol:

  • It supports buffer transfer only; there is no real controlling of the follower side from the conductor side, other than configuration values that can be written.
  • Choosing the conductor and follower side is not explicitly supported.

These are the specific limitations I've encountered that result from these:

Conductor (USB host-connected) side detection

The SPLIT_USB_DETECT side-detection mechanism requires that the follower side time out from detecting an active USB connection to decide to become the follower. During this timeout (which defaults to two seconds), the keyboard is unresponsive (the conductor side hangs while trying to communicate with its peer). Shortening the timeout, though, would make follower side detection less reliable.

This period of inactivity at startup prevents using the keyboard to trigger alternate modes in the host computer as it boots up. This means always having another keyboard around when doing boot-time configuration.

Sleep/wake

The follower side doesn't go into low-power mode when the conductor does; there's nothing to tell it to do so. The closest we come is disabling the RGB lights on the follower side by clearing the enable bit and sending it over if RGBLIGHT_SLEEP is defined. I'm not even sure if the split transport slows down.

RGB lighting

There have been some great improvements recently in managing the RGB light state and animations over the two halves when RGBLIGHT_SPLIT, but synchronization is still imperfect. For instance unless RGBLIGHT_SLEEP is defined, then the animation pauses on the conductor side, but continues on the follower side. Further, when the conductor awakes, the two animations will be out of sync. (PR #10997 should help address this, though.)

Prior art

I'm not the first to realize that the startup timeout could be eliminated if the conductor side (which discovers that it's the conductor exactly as soon as it is able to communicate with the host) were able to tell the follower side to become the follower side.

And we need to be careful of initialization races:

In particular, it's important that the follower not initialize things like its RGB parameters after responding to the RGB sync messages over the split protocol, or the EEPROM settings will end up overriding those sent from the conductor.

Proposal

I propose the an improved split transport protocol that aims to solve all of these problems.

Add new states

In order to bring the conductor and follower sides up in a quick, orderly fashion, I believe we need to introduce a couple new states, extending the current pair of is_master and !is_master:

  • PENDING (also thought of calling this UNDIFFERENTIATED)
  • CONDUCTOR
  • BECOMING_FOLLOWER
  • FOLLOWER

Each half starts in PENDING state. Only certain state transitions are allowed:

  • PENDING -> CONDUCTOR
    • When the side achieves USB connectivity to the host
  • PENDING -> BECOMING_FOLLOWER
    • When instructed by the conductor side to become the follower
  • BECOMING_FOLLOWER -> FOLLOWER
    • After initializing follower state

There isn't a need for a BECOMING_CONDUCTOR state, since all the initialization on the conductor side can happen without interference from the other side.

There is also no need to fall back to prior states; only USB disconnection can accomplish that.

Here are the rules in each state:

  • PENDING

Each side must initialize its split transport as if it were a follower in order to listen for the "become_follower" message. Neither side is allowed to send any messages on the split transport.

  • CONDUCTOR

When the side achieves a USB connection, it initializes itself and transitions to this state, and then sends the "become_follower" message. It will then begin attempting to communicate with the follower side, although it needs to be prepared for the follower side not to respond for a few milliseconds.

  • BECOMING_FOLLOWER

The side transitions to this state when it receives the "become_follower" message. In this state, the side does not respond to any messages on the split transport. (It should either shut off the interrupt on the transport pin(s) in this transitional state or wrap the code in the interrupt handler with if (side_state == FOLLOWER).) It initializes itself and transitions to FOLLOWER.

  • FOLLOWER

In this state, the follower side responds to messages over the split transport. The full keyboard is now operational.

Add commands in addition to just buffer transfers

The "become_follower" message mentioned above is a new kind of transfer over the split transport: a command. The following commands are to be defined:

  • Become_follower
  • Suspend
    • Shut off the RGB lights, and otherwise reduce power use as much as feasible
  • Wake
  • Sync_RGB
    • Syncs RGB light animation to start of sequence

Each of these commands is sent as a single byte that the follower ACKs; no data need be transferred.

In the serial transport, this extends the meaning of the first byte sent. I propose that all commands have the high bit set, and anything with the high bit unset will be treated as an sstd_index.

In the I2C transport, a new single-byte register could be used to receive commands, and any time a command byte is written to that register, then the follower would act on it.

In both cases, care must be taken to exit interrupt state as appropriate. One way to accomplish this would be to have the interrupt servicing routine just add the command to a short queue to be handled on the next iteration of the main service loop.

Additional refinements

I also think the transport code could have some additional abstractions added to make the I2C and serial transports less different from the perspective of quantum/split_common/transport.c. For instance, a slightly higher-level exchange_buffers(buffer_identifier) could wrap both soft_serial_transaction() and i2c_writeReg() if carefully designed. And a new send_command(command_num) could be added.

It would also be great to improve handling of NACKs or failures to ACK. The current implementation experiences significant timeouts in abnormal situations (such as one half missing or unresponsive) that render the conductor side unable to scan its matrix.

It will similarly be important to handle gracefully the time during which the other side is in BECOMING_FOLLOWER.

Implementation

If this proposal is sensible, then I will be happy to implement it, targeting the develop branch (since it will change the protocol and require flashing both halves).

I believe that this should be a straightforward extension of the current split transport protocol, and so it should (after thorough testing) be able to replace the current protocol in the code.

The actual bits on the wire will not be substantially different, with the exception of the (fairly rare) sending of the command words, so there shouldn't be much impact on either the binary size or protocol performance.

I am curious how important it is to retain the current non-SERIAL_USE_MULTI_TRANSACTION code. I think there is, in some cases, a one-byte difference in the data sent on the wire, but the API itself mostly hides the implementation difference. Are there any devices in existence that would be hurt by being "upgraded" to the SERIAL_USE_MULTI_TRANSACTION protocol or, indeed, this extended protocol, the next time their firmware is recompiled?

@sturem
Copy link

sturem commented Mar 22, 2021

Just a couple of notes:

  1. I've also seen "leader" and "follower" used, versus "conductor" and "follower"; both are clear, though (and sometimes "primary" and "secondary" are used, with similar meaning);

  2. Should a "Return to PENDING/UNDIFFERENTIATED" command also be defined, for when the USB is unplugged?
    Especially if unplugging/replugging the USB from one side to the other should be supported (and/or could one support a key command to force Left or Right as Conductor/Leader/Primary, with both USBs plugged in to different computers? Or could this cause issues, e.g. with 5v power? Maybe related to issue#13231 ? for Drop CRTL)

  3. Pls edit the issue to set the title at:
    [Feature Proposal] [Feature Proposal]  #12033

@stale
Copy link

stale bot commented Jun 23, 2021

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs.
For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

@stale stale bot added the stale Issues or pull requests that have become inactive without resolution. label Jun 23, 2021
@stale
Copy link

stale bot commented Jul 23, 2021

This issue has been automatically closed because it has not had activity in the last 30 days. If this issue is still valid, re-open the issue and let us know.

@stale stale bot closed this as completed Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

No branches or pull requests

2 participants