Removed the need for timeout if using SPLIT_USB_DETECT #9742

elagil · 2020-07-17T08:56:33Z

Following my issue #8990, I created a fix for the described problem. To reiterate:

With SPLIT_USB_DETECT, QMK currently tries to find a USB connection during a timeout period (default 2 seconds). If not found, the board half is set to slave, otherwise to master. This is unreliable, if the connection is not found within the timeout period for some reason. Now, the detection is much faster and reliable. It is not required to wait for any timeout to finish.

Description

I implemented a communication phase during the USB master side detection, which works like this:

Both halves are initialized as slaves w.r.t. the transport that is used
Both halves check for USB connection indefinitely, until one of them finds it
The USB-connected half tells the other half, that it found the connection and thus becomes the master
The slave half then stops looking for USB and becomes the slave

I tested for UART but implemented changes for I2C as well. I would be happy, if someone could test it, since I have no I2C board available.

Types of Changes

Issues Fixed or Closed by This PR

#8990

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

Due to the change in behaviour, this should target the develop branch.

zvecr · 2020-07-17T09:04:31Z

quantum/split_common/split_util.c

@@ -135,6 +132,9 @@ __attribute__((weak)) bool is_keyboard_master(void) {
 void split_pre_init(void) {
    isLeftHand = is_keyboard_left();

+    // Start in slave configuration and wait for a master to emerge
+    transport_slave_init();


Adding this here now calls init twice, and breaks #8235 again.

You mean, it's called twice for the master half?

I guess, for changing the target branch without strange side effects, I need to reopen this.

no slave half https://github.com/qmk/qmk_firmware/blob/72e522bfc6c98f0710b1c2fb615b600730aa96cb/quantum/split_common/split_util.c#L158-L162

Do you think that if I just remove the repeated initialization in split_post_init, it will bring back the race-condition bug? The slave will finish USB initialization when the master is determined and will not overwrite any settings that are sent over the transport.

quantum/split_common/transport.c

zvecr · 2020-07-17T09:23:39Z

quantum/split_common/split_util.c

@@ -135,6 +132,9 @@ __attribute__((weak)) bool is_keyboard_master(void) {
 void split_pre_init(void) {
    isLeftHand = is_keyboard_left();


This will infinite loop on MASTER_LEFT/MASTER_RIGHT as the slave transport is not established.

Can you explain? Do you mean that it will not return as long as there is no slave that answers the master's messages?

Edit: Also, is that a problem? I just tested with my UART split board and the master does not stay in that loop after connecting USB. It works without a slave as well.

I just tested with my UART split board and the master does not stay in that loop after connecting USB. It works without a slave as well.

This is a board that uses EE_HANDS/SPLIT_HAND_PIN for handedness?

Yes, it is (EE_HANDS). Let's say it was a board that entered is_keyboard_master(), this would return after finding USB or receiving a message from the master. Am I missing something?

in that case is_keyboard_left calls is_keyboard_master before transport_slave_init happens. Thus it will never get its "slave" assignment as there will never be anything listening.

I moved the slave initialization forward in a later commit (ea40f38): https://github.com/qmk/qmk_firmware/pull/9742/files#diff-57e6bd7db0b3e56c607994b241d7b945R132-R134

elagil · 2020-07-17T10:37:52Z

I did some cleanup for rebasing this onto the develop branch. Excuse the repeated force-pushes.

Edit: Force-push to the develop branch required fixing again.

elagil · 2020-07-17T14:51:43Z

I managed to remove the review request for qmk/collaborators, sorry for that.

elagil · 2020-07-24T10:38:31Z

@zvecr For pushing this forward, do you see any further issues with the current state of the code? Sadly, I cannot test the operation for I2C.

zvecr · 2020-07-26T21:31:00Z

Yes, #8235 would still be re-broken by this change.

elagil · 2020-07-27T05:54:19Z

Yes, #8235 would still be re-broken by this change.

I am failing to see why that would be the case. Let me summarize the new order of events upon initialization:

USB Master switches on, is initialized as a slave w.r.t. transport
Slave switches on, is initialized as a slave, too. Probably reads EEPROM for RGB settings at this point. No further initialization is done on the slave beyond this point.
Master finds USB, is finally initialized as master w.r.t transport
Master shares RGB information with the slave, and tells it that USB was found. Slave takes these settings.
Initialization finished

I don't see where the slave overwrites the RGB settings that it got from the master via transport with those old settings from EEPROM. The slave is initialized at the beginning in 2), prior to receiving new RGB settings. If it is initialized after the master has already found USB, then the serial transfer of new settings failswell and will be repeated. Therefore, no race condition can happen, as far as I can understand.

To reiterate: there is only one initialization on the slave side, before any communication with the master can occur. Therefore, initialization (and reading old settings from EEPROM) cannot happen after a link to the master is successfully established.

noroadsleft · 2020-08-12T16:05:28Z

This needs to be rebased, and the merge conflicts resolved.

elagil · 2020-08-15T14:46:48Z

This needs to be rebased, and the merge conflicts resolved.

I will do it next week.

elagil · 2020-08-16T17:16:25Z

@noroadsleft Rebased to latest develop force-push.

zvecr

#8235 describes the issue. The main part is that slave is listening during startup, so will respond to any rgb sync actions before its ready to do so. Also, the listening on the master side being left on would also mean that serial commands would still trigger the ISR processing on both halves.

elagil · 2020-08-30T16:56:47Z

@zvecr I don't quite get how communication works. Could you clarify a bit?

I see that in 'matrix_post_scan()', the 'transport_master()' and 'transport_slave()' functions are called. These handle any data that was shared over the transport (UART or I2C). I am on AVR at the moment, so this is not ChibiOS-based, right? Can you point me to the interrupt handler that you are talking about? I only find one for ChibiOS.

With regard to leaving the slave interrupt handlers enabled on the master-side, I will look into disabling them after the master was found.

elagil · 2020-11-29T09:36:04Z

@tzarc Should further development continue on this issue?

tzarc · 2020-11-30T09:12:14Z

I was in the process of reopening accidentally closed PRs -- we went through a develop -> master merge and the source branch was accidentally deleted, closing all open PRs at the time of deletion. Was just reopening yours so that it can proceed. This was one of about 80 that were reopened.

elagil · 2020-11-30T09:38:50Z

I understand!

drashna · 2021-03-14T19:28:39Z

This has a bunch of merge conflicts, now.

elagil · 2021-03-27T16:52:24Z

This has a bunch of merge conflicts, now.

I will address them soon. @drashna Maybe it makes sense to work on a unified PR according to #12033?

stale · 2022-01-03T21:45:07Z

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

elagil · 2022-04-09T07:51:33Z

Closing, out of date.

GilDev · 2023-10-11T07:54:18Z

@elagil Hey, is it out of date because it has been fixed elsewhere somehow? Thanks!

elagil · 2023-10-12T14:56:49Z

@GilDev I don't know if it was fixed, but my contribution is no longer compatible with the current codebase (and was never merged).

zvecr requested changes Jul 17, 2020

View reviewed changes

zvecr added core enhancement labels Jul 17, 2020

zvecr requested a review from a team July 17, 2020 09:06

elagil changed the base branch from master to develop July 17, 2020 09:13

elagil changed the base branch from develop to master July 17, 2020 09:13

zvecr requested changes Jul 17, 2020

View reviewed changes

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

zvecr changed the base branch from master to develop July 17, 2020 09:24

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

elagil requested review from zvecr and removed request for a team July 17, 2020 14:43

zvecr requested a review from a team July 17, 2020 16:35

noroadsleft force-pushed the develop branch from 415f088 to 14610ee Compare July 18, 2020 07:31

noroadsleft force-pushed the develop branch from 7d7f5bf to 2302d3b Compare July 26, 2020 07:32

noroadsleft force-pushed the develop branch from 550bb6a to 9d827f9 Compare July 31, 2020 19:44

noroadsleft force-pushed the develop branch from 7499550 to 8b7b3ce Compare August 11, 2020 08:24

zvecr requested changes Aug 16, 2020

View reviewed changes

noroadsleft force-pushed the develop branch 2 times, most recently from 1d4c0cd to 65c27a3 Compare August 29, 2020 21:41

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

This was referenced Sep 18, 2020

Add possibility of forcing the master side for split boards #10352

Closed

Enable forcing of master side for split boards #10353

Closed

skullydazed force-pushed the develop branch from fa8cf59 to cce70f1 Compare October 18, 2020 21:10

noroadsleft force-pushed the develop branch from 5720202 to dcefc1a Compare October 23, 2020 22:37

skullydazed force-pushed the develop branch from db745a9 to 68fc044 Compare October 28, 2020 02:16

noroadsleft force-pushed the develop branch from 92ca6b9 to c3221d4 Compare October 30, 2020 20:23

elagil mentioned this pull request Nov 11, 2020

[Bug] SPLIT_USB_DETECT prevents keyboard detection on cold boot #8990

Closed

noroadsleft closed this Nov 28, 2020

noroadsleft deleted the branch qmk:develop November 28, 2020 20:02

tzarc reopened this Nov 28, 2020

noroadsleft force-pushed the develop branch from 7d3841e to 3cc7d22 Compare November 30, 2020 11:32

gsadams mentioned this pull request Feb 27, 2021

[Feature Proposal] #12033

Closed

tzarc closed this Feb 27, 2021

tzarc deleted the branch qmk:develop February 27, 2021 20:27

noroadsleft reopened this Feb 27, 2021

tzarc added stale Issues or pull requests that have become inactive without resolution. and removed breaking_change Changes that need to wait for a version increment labels Nov 1, 2021

stale bot added the awaiting changes label Jan 3, 2022

stale bot removed the stale Issues or pull requests that have become inactive without resolution. label Apr 9, 2022

elagil closed this Apr 9, 2022

elagil deleted the split_enhancement branch April 9, 2022 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed the need for timeout if using SPLIT_USB_DETECT #9742

Removed the need for timeout if using SPLIT_USB_DETECT #9742

elagil commented Jul 17, 2020

zvecr left a comment

zvecr Jul 17, 2020

elagil Jul 17, 2020

elagil Jul 17, 2020

zvecr Jul 17, 2020

elagil Jul 17, 2020

zvecr Jul 17, 2020

elagil Jul 17, 2020 •

edited

Loading

zvecr Jul 17, 2020

elagil Jul 17, 2020 •

edited

Loading

zvecr Jul 17, 2020

elagil Jul 17, 2020 •

edited

Loading

elagil commented Jul 17, 2020 •

edited

Loading

elagil commented Jul 17, 2020

elagil commented Jul 24, 2020

zvecr commented Jul 26, 2020 •

edited

Loading

elagil commented Jul 27, 2020 •

edited

Loading

noroadsleft commented Aug 12, 2020

elagil commented Aug 15, 2020

elagil commented Aug 16, 2020

zvecr left a comment

elagil commented Aug 30, 2020

elagil commented Nov 29, 2020

tzarc commented Nov 30, 2020

elagil commented Nov 30, 2020

drashna commented Mar 14, 2021

elagil commented Mar 27, 2021 •

edited

Loading

stale bot commented Jan 3, 2022

elagil commented Apr 9, 2022

GilDev commented Oct 11, 2023

elagil commented Oct 12, 2023

		@@ -135,6 +132,9 @@ __attribute__((weak)) bool is_keyboard_master(void) {
		void split_pre_init(void) {
		isLeftHand = is_keyboard_left();

Removed the need for timeout if using SPLIT_USB_DETECT #9742

Removed the need for timeout if using SPLIT_USB_DETECT #9742

Conversation

elagil commented Jul 17, 2020

Description

Types of Changes

Issues Fixed or Closed by This PR

Checklist

zvecr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elagil Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elagil Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elagil Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

elagil commented Jul 17, 2020 • edited Loading

elagil commented Jul 17, 2020

elagil commented Jul 24, 2020

zvecr commented Jul 26, 2020 • edited Loading

elagil commented Jul 27, 2020 • edited Loading

noroadsleft commented Aug 12, 2020

elagil commented Aug 15, 2020

elagil commented Aug 16, 2020

zvecr left a comment

Choose a reason for hiding this comment

elagil commented Aug 30, 2020

elagil commented Nov 29, 2020

tzarc commented Nov 30, 2020

elagil commented Nov 30, 2020

drashna commented Mar 14, 2021

elagil commented Mar 27, 2021 • edited Loading

stale bot commented Jan 3, 2022

elagil commented Apr 9, 2022

GilDev commented Oct 11, 2023

elagil commented Oct 12, 2023

elagil Jul 17, 2020 •

edited

Loading

elagil Jul 17, 2020 •

edited

Loading

elagil Jul 17, 2020 •

edited

Loading

elagil commented Jul 17, 2020 •

edited

Loading

zvecr commented Jul 26, 2020 •

edited

Loading

elagil commented Jul 27, 2020 •

edited

Loading

elagil commented Mar 27, 2021 •

edited

Loading