Port SPLIT_USB_DETECT to helix/rev2 #7385

stormbard · 2019-11-17T03:30:36Z

Description

This PR adds SPLIT_USB_DETECT support for those users having issues with Elite-C V3s.
Copied from PR #7195

Types of Changes

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

~~void matrix_init(void) within matrix.c also needs renaming to matrix_setup~~ scratch that, just the block of debug toggles needs to be removed:

void matrix_init(void)
{
-    debug_enable = true;
-    debug_matrix = true;
-    debug_mouse = true;

zvecr · 2019-11-17T04:18:12Z

Tagging @mtei @MakotoKurauchi
While there are long term plans to move to split_common, any issues with this change? I will really help with some of the reported issues we are seeing!

mtei · 2019-11-17T04:45:28Z

Currently, I am suggesting to MakotoKurauch to change Helix to use oled_driver.c instead of ssd1306.h. At the same time, I am preparing for the transition to split_common.
MakotoKurauchi#24

This PR may conflict with the above changes. So please don't do this PR yet.

mtei · 2019-11-17T05:18:00Z

This PR adds bool is_keyboard_master(void) to keyboards/helix/rev2/split_util.c.
Doing so will cause problems with the operation of RGB light. The right half rgb light mode will not be changed.

zvecr · 2019-11-17T11:37:05Z

@mtei There are users currently suffering issues so I would rather push for this to go in.

split_common generally needs extra work to make it compatible, such as syncing matrix state. This will take extra time to review and merge. (Also that PR changes user keymaps so would have to go through the breaking change process). While this PR is based off another short term fix we have already applied.

As for adding is_keyboard_master, that wont break anything within rgblight (its not like we are in a language where we can check if the symbol is defined).

mtei · 2019-11-17T13:07:59Z

As for adding is_keyboard_master, that wont break anything within rgblight (its not like we are in a language where we can check if the symbol is defined).

Now I did the following:

git fetch qmk pull/7385/head:x
git checkout x
make helix/rev2/back:five_rows:avrdude-loop

And I tried RGB_TOG key, RGB_MOD key.
The master side worked correctly.
On the slave side, the LED cannot be turned on/off. mode does not change.

zvecr · 2019-11-17T13:15:26Z

Is it this block that is in play then?

qmk_firmware/tmk_core/common/keyboard.c

Lines 288 to 316 in 46d0fe4

    
               if (is_keyboard_master()) { 
        
                   for (uint8_t r = 0; r < MATRIX_ROWS; r++) { 
        
                       matrix_row    = matrix_get_row(r); 
        
                       matrix_change = matrix_row ^ matrix_prev[r]; 
        
                       if (matrix_change) { 
        
           #ifdef MATRIX_HAS_GHOST 
        
                           if (has_ghost_in_row(r, matrix_row)) { 
        
                               continue; 
        
                           } 
        
           #endif 
        
                           if (debug_matrix) matrix_print(); 
        
                           for (uint8_t c = 0; c < MATRIX_COLS; c++) { 
        
                               if (matrix_change & ((matrix_row_t)1 << c)) { 
        
                                   action_exec((keyevent_t){ 
        
                                       .key = (keypos_t){.row = r, .col = c}, .pressed = (matrix_row & ((matrix_row_t)1 << c)), .time = (timer_read() | 1) /* time should not be 0 */ 
        
                                   }); 
        
                                   // record a processed key 
        
                                   matrix_prev[r] ^= ((matrix_row_t)1 << c); 
        
           #ifdef QMK_KEYS_PER_SCAN 
        
                                   // only jump out if we have processed "enough" keys. 
        
                                   if (++keys_processed >= QMK_KEYS_PER_SCAN) 
        
           #endif 
        
                                       // process a key per task call 
        
                                       goto MATRIX_LOOP_END; 
        
                               } 
        
                           } 
        
                       } 
        
                   } 
        
               }

Where the default implementation of is_master currently always returns true.

If so, we can just rename is_keyboard_master, maybe back to has_usb.

zvecr · 2019-11-19T02:57:53Z

@stormbard can you copy in the latest updated found within #7404, this should fix keycode processing and potentially unblock this PR.

stormbard · 2019-11-19T18:55:54Z

@zvecr I can push the changes but in my testing locally on my helix the right hand LEDs do not toggle off when the toggle key is pressed. Doesn't matter which one is master in this case.

I'm also still seeing some intermittent issues with the halves communicating. I'm not sure if this is due to a firmware or hardware issue though and will take suggestions for debugging.

zvecr · 2019-11-22T23:51:47Z

It sounds like a hardware issue, though I don't have anything on hand to verify that is the case.

Does reverting the suggestions within #7404 make it work again?

stormbard · 2019-11-23T01:04:30Z

Didn't work before or after the changes in #7404.

stormbard · 2019-12-02T19:23:25Z

After some further testing I believe this change is good. I'm able to get the backlighting to work on both halves as I'd expect.

mtei · 2019-12-05T15:37:10Z

I made the patch to be a better implementation than I think.
~~mtei@61350f4~~
~~mtei@1ba76da~~
mtei@8bd374e
Would you like try it?

edit:
This patch includes the following changes:

keyboards/helix/rev2/matrix.c
moved uint8_t is_master; to keyboards/helix/rev2/rev2.c.
keyboards/helix/rev2/split_util.[ch]
- Renamed has_usb() to a more appropriately named is_helix_master().
- Removed __attribute__ ((weak)) since it should not be used here.
keyboards/helix/rev2/rev2.c
Added is_master definition and initialization for backward compatibility.
keyboards/helix/rev2/rev2.h
- Added is_keyboard_master() as a wrapper macro.
- Added has_usb() as a wrapper macro for backward compatibility.
~~keyboards/helix/rev2/rules.mk~~.
~~Make serial.c compile properly when LTO_ENABLE = yes.~~
~~users/xulkal/custom_oled.c~~.
~~Added is_keyboard_master() wrapper macro like keyboards/helix/rev2/rev2.h.~~

~~The 'xulkal' keymap needs to be changed to 5 and 6 above.~~

The 'xulkal' keymap requires is_keyboard_master(). However, if is_keyboard_master() is implemented as a function, 'tmk_core/common/keyboard.c:keyboard_task()' will behave differently between master and slave. This means that helix's split_utils.c, which expects to do the same for both master and slave, will not work properly.

To solve this, is_keyboard_master() must be implemented as a macro so that it is not called from 'tmk_core/common/keyboard.c:keyboard_task()'.

'users/xulkal/custom_oled.c' was outside of 'keyboards/helix/rev2', but a similar situation can occur in the source code in 'keyboards/helix/rev2'. Therefore, is_keyboard_master() is implemented as a macro in 'helix/rev2/rev2.h'.

zvecr · 2019-12-05T16:37:59Z

the 'xulkal' keymap was not working.

@mtei Can you elaborate further on what the issue is please. The keymap compiles fine, and that suggestion is rather involved to see what the actual problem is.

XScorpion2 · 2019-12-05T16:44:06Z

You can probably ignore, 'xulkal', helix keymap, I haven't used that board in a while, and usually have a quick turn around on fixing my own keymaps. Though I always do appreciate a heads up when one of my keymaps break.

Actually, I see exactly why my keymap breaks, there's a wrapper function I implemented in my user space for switching between split common, and custom split like the helix and I suspect that is now returning the wrong values. Easy fix.

zvecr · 2019-12-06T12:19:44Z

@mtei I don't see any of those changes being a hard requirement for this PR:

removing has_usb potentially breaks backwards compatibility
moving is_master just adds code churn, and these short term fixes tend to aim for minimal change
adding is_keyboard_master goes back to Fix processing of RGB keycodes on crkbd slave half #7404 where it will break previous assumptions, though as per that PR, its only an issue if included in tmk_core/common/keyboard.c which the proposed rev2.h currently isn't unless a user adds in within their keymaps config.h path
4 serial.c and its compilation in this PR remains unchanged so should be considered out of scope
xulkal keymaps implementation of is_keyboard_master should remain unchanged as the underpinning is_master bool it depends on has the same behaviour as before

Though if you do want to propose some of those changes, feel free to raise a separate PR.

mtei · 2019-12-07T09:53:16Z

adding is_keyboard_master goes back to Fix processing of RGB keycodes on crkbd slave half #7404 where it will break previous assumptions, though as per that PR, its only an issue if included in tmk_core/common/keyboard.c which the proposed rev2.h currently isn't unless a user adds in within their keymaps config.h path
4 serial.c and its compilation in this PR remains unchanged so should be considered out of scope

I'm not good at English. This sentence was too long for me to understand. So I can't comment, sorry.

However, I have confirmed that my patch has been tested and works correctly with all keymaps. Don't forget it.

xulkal keymaps implementation of is_keyboard_master should remain unchanged as the underpinning is_master bool it depends on has the same behaviour as before

'helix:xulkal' has problems similar to those solved by #7404. Currently, 'helix:xulkal' can be compiled but is not working properly. If you want 'helix:xulkal' to work correctly, you need to fix it.

I checked the operation before and after applying my patch. It has been confirmed that it will not work properly before the patch is applied. It has been confirmed that it works correctly after applying the patch.

However, the 'helix:xulkal' problem is certainly outside the scope of this PR.
I will open a new PR after this PR has been merged.

mtei · 2019-12-07T09:53:26Z

Of course I don't care if my patch was not included in this PR. Either way, I approve this PR. However, my patch above will be needed for backward compatibility when Helix changes to use split_common in the future.

removing has_usb potentially breaks backwards compatibility

moving is_master just adds code churn, and these short term fixes tend to aim for minimal change

'is_master' and 'has_usb()' must not be removed for backward compatibility. You say my patch breaks backward compatibility, which is a misunderstanding. My patch keeps backward compatibility better than the current code of this PR.

The current code for this PR causes problems when changing Helix to use split_common. My patch avoids that problem.

zvecr · 2019-12-07T11:48:38Z

My patch keeps backward compatibility better than the current code of this PR

That is a bit of an abstract statement, can you provide an example where this PR breaks backwards compatibility?

mtei · 2019-12-07T12:07:10Z

To reiterate what I already wrote above, in this PR's current code, 'is_master' is in 'helix/rev2/matrix.c' and 'has_usb()' is in 'helix/It is in rev2/split_util.c'.
If Helix uses split_common, the two files will not be linked.

zvecr · 2019-12-07T12:17:04Z

But thats not a backwards compatibility issue.

As there are none being raised, im going to merge this as is.

zvecr · 2019-12-07T12:17:25Z

Cheers for your work @stormbard!

* Port SPLIT_USB_DETECT to helix/rev2 * Remove debug toggles. * Rename is_keyboard_master to has_usb in split_util

Port SPLIT_USB_DETECT to helix/rev2

de61ad2

zvecr requested changes Nov 17, 2019

View reviewed changes

zvecr added the keyboard label Nov 17, 2019

zvecr requested a review from a team November 17, 2019 04:15

Remove debug toggles.

d4f275c

zvecr approved these changes Nov 17, 2019

View reviewed changes

drashna approved these changes Nov 17, 2019

View reviewed changes

zvecr mentioned this pull request Nov 19, 2019

Fix processing of RGB keycodes on crkbd slave half #7404

Merged

13 tasks

Rename is_keyboard_master to has_usb in split_util

a6039f5

zvecr approved these changes Dec 2, 2019

View reviewed changes

zvecr merged commit 36a6e26 into qmk:master Dec 7, 2019

stormbard deleted the helix_usb_detect branch December 8, 2019 04:20

patrl pushed a commit to patrl/qmk_firmware that referenced this pull request Dec 29, 2019

Port SPLIT_USB_DETECT to helix/rev2 (qmk#7385)

c3824f9

* Port SPLIT_USB_DETECT to helix/rev2 * Remove debug toggles. * Rename is_keyboard_master to has_usb in split_util

zvecr mentioned this pull request Feb 6, 2020

Port SPLIT_USB_DETECT to lily58 #8107

Merged

13 tasks

HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020

Port SPLIT_USB_DETECT to helix/rev2 (qmk#7385)

b1499ff

* Port SPLIT_USB_DETECT to helix/rev2 * Remove debug toggles. * Rename is_keyboard_master to has_usb in split_util

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port SPLIT_USB_DETECT to helix/rev2 #7385

Port SPLIT_USB_DETECT to helix/rev2 #7385

stormbard commented Nov 17, 2019

zvecr left a comment •

edited

Loading

zvecr commented Nov 17, 2019

mtei commented Nov 17, 2019 •

edited

Loading

mtei commented Nov 17, 2019

zvecr commented Nov 17, 2019 •

edited

Loading

mtei commented Nov 17, 2019

zvecr commented Nov 17, 2019 •

edited

Loading

zvecr commented Nov 19, 2019

stormbard commented Nov 19, 2019

zvecr commented Nov 22, 2019

stormbard commented Nov 23, 2019

stormbard commented Dec 2, 2019

mtei commented Dec 5, 2019 •

edited

Loading

zvecr commented Dec 5, 2019 •

edited

Loading

XScorpion2 commented Dec 5, 2019 •

edited

Loading

zvecr commented Dec 6, 2019 •

edited

Loading

mtei commented Dec 7, 2019

mtei commented Dec 7, 2019

zvecr commented Dec 7, 2019 •

edited

Loading

mtei commented Dec 7, 2019

zvecr commented Dec 7, 2019

zvecr commented Dec 7, 2019

Port SPLIT_USB_DETECT to helix/rev2 #7385

Port SPLIT_USB_DETECT to helix/rev2 #7385

Conversation

stormbard commented Nov 17, 2019

Description

Types of Changes

Checklist

zvecr left a comment • edited Loading

Choose a reason for hiding this comment

zvecr commented Nov 17, 2019

mtei commented Nov 17, 2019 • edited Loading

mtei commented Nov 17, 2019

zvecr commented Nov 17, 2019 • edited Loading

mtei commented Nov 17, 2019

zvecr commented Nov 17, 2019 • edited Loading

zvecr commented Nov 19, 2019

stormbard commented Nov 19, 2019

zvecr commented Nov 22, 2019

stormbard commented Nov 23, 2019

stormbard commented Dec 2, 2019

mtei commented Dec 5, 2019 • edited Loading

zvecr commented Dec 5, 2019 • edited Loading

XScorpion2 commented Dec 5, 2019 • edited Loading

zvecr commented Dec 6, 2019 • edited Loading

mtei commented Dec 7, 2019

mtei commented Dec 7, 2019

zvecr commented Dec 7, 2019 • edited Loading

mtei commented Dec 7, 2019

zvecr commented Dec 7, 2019

zvecr commented Dec 7, 2019

zvecr left a comment •

edited

Loading

mtei commented Nov 17, 2019 •

edited

Loading

zvecr commented Nov 17, 2019 •

edited

Loading

zvecr commented Nov 17, 2019 •

edited

Loading

mtei commented Dec 5, 2019 •

edited

Loading

zvecr commented Dec 5, 2019 •

edited

Loading

XScorpion2 commented Dec 5, 2019 •

edited

Loading

zvecr commented Dec 6, 2019 •

edited

Loading

zvecr commented Dec 7, 2019 •

edited

Loading