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

Add VIA support to MxSS, plus minor tweaks #7809

Merged
merged 24 commits into from Jan 26, 2020
Merged

Conversation

MxBlu
Copy link
Contributor

@MxBlu MxBlu commented Jan 6, 2020

As it says on the label. Added a universal layout, did a bit of cleanup and added support for RGB Test effect mode while I was at it. Those changes are like only a few lines long, which is why I've combined them into this one, I hope that's alright.

VIA PR: the-via/keyboards#20

Types of Changes

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

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

@noroadsleft
Copy link
Member

@Wilba6582 If you could check over the VIA implementation here, it'd be much appreciated.

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 6, 2020

I followed the method that @awkannan did for the Satisfaction75 here, so things should align with the intended implementation.

@MxBlu MxBlu mentioned this pull request Jan 6, 2020
7 tasks
Copy link
Contributor

@wilba wilba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EEPROM handling could be refactored to be closer to how I did it during the VIA refactor, examples: keyboards\cannonkeys\stm32f072\keyboard.c , keyboards\wilba_tech\wt_main.c.

i.e. if VIA not enabled, matrix_init_kb() calls via_init() which calls via_eeprom_*() functions.

keyboards/mxss/mxss.c Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.h Outdated Show resolved Hide resolved
@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 7, 2020

Advice followed.

@drashna drashna requested review from drashna and a team January 8, 2020 05:28
@wilba
Copy link
Contributor

wilba commented Jan 8, 2020

👍

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 8, 2020

While I was waiting, I happened to realise that RGB was also an option... This lot of commits shuffle around my code and add in VIA RGB API handlers based on @fcoury's PR here.

@zvecr
Copy link
Member

zvecr commented Jan 8, 2020

While I was waiting, I happened to realise that RGB was also an option... This lot of commits shuffle around my code and add in VIA RGB API handlers based on @fcoury's PR here.

Given that #4915 targets old via code, and isnt merged. I dont think we should be adding the mentioned handlers unless they work out of the box right now.

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 8, 2020

They work out of the box. My work was to update it to extend on the commit to support more of the API.

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 8, 2020

Oh, if the concern was whether my VIA API is up to date, I was referred to that PR by olivia who asked me to test if it worked, and it does out of the box. The current VIA implementation expects the keyboard to handle backlight related functions, and I pulled the list of API calls from the VIA repo (though they have not been updated since anyhow).

@wilba
Copy link
Contributor

wilba commented Jan 9, 2020

@MxBlu I gave my 👍 before all this RGB stuff.

We are working on a generalized implementation for the QMK RGB (bottom LEDs and/or matrix). It would make more sense to wait for this, than end up in another situation where I have to refactor other people's code to make it continue to work in VIA.

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 9, 2020

A shame, but alright. Will revert the merge, but leave my code up on https://github.com/MxBlu/qmk_firmware/tree/mxss-via-backlight.
FYI, this was my attempt at a generalised solution (if you pull out the indicator LED code), but I'll leave you to engineer something a bit more robust.

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 9, 2020

Alright, this lot of changes is just a cherrypick from the backlight branch, which updates my rgblight.c to the current one, and merges in my changes. Also has some quality of life things like shifting over the front LED code to another source file, and moving some parts of color storage to EEPROM in prep for being able to add in VIA RGB support in the future.

I apologise for it being a bit overwhelming, but tidying it now will make it easier in the future.

@drashna drashna requested a review from a team January 9, 2020 20:03
keyboards/mxss/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/mxss/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.h Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.h Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.h Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.c Show resolved Hide resolved
keyboards/mxss/mxss_frontled.h Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.h Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.c Outdated Show resolved Hide resolved
keyboards/mxss/mxss_frontled.c Outdated Show resolved Hide resolved
@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 16, 2020

@fauxpark Changes have been addressed.

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 18, 2020

Anything more to be done to get this merged in?

@MxBlu
Copy link
Contributor Author

MxBlu commented Jan 26, 2020

Can I please get some feedback/get this merged...?

@ridingqwerty
Copy link
Contributor

Given that Wilba, Fauxpark, and Drashna have all approved, I’d say this is good to merge.

@ridingqwerty ridingqwerty merged commit 645c5fa into qmk:master Jan 26, 2020
HokieGeek pushed a commit to HokieGeek/qmk_firmware that referenced this pull request Feb 21, 2020
* Add VIA support to mxss and general cleanup

* Add support for RGB test for FLEDs

* Add LAYOUT_all to allow for more configuration

* Remove blank layers

* Updated readme

* Improve use of EEPROM

* Credit where its due

* Use the latest iteration of rgblight code

* Keep the RGB timer running if the front LED is in RGB mode

* Fix RGB breathing animation

* Better supported RGB animation
Only thing not working is alternating, but that's not too important

* Abstract front LED handlers from main kb code

* Add support for indicator LED color changing

* Remove debug statement

* Persist indicator LED colors

* Mark custom sections in rgblight.c

* Light commenting

* Fix up keymaps

* Add/update comments

* Remove bloat from default hex

* Tidy a stray tab

* Out with the old, in with the new

* Out with the old, in with the new

* Add LAYER_STATE_8BIT for VIA keymap
c0psrul3 pushed a commit to c0psrul3/qmk_firmware that referenced this pull request Mar 23, 2020
* Add VIA support to mxss and general cleanup

* Add support for RGB test for FLEDs

* Add LAYOUT_all to allow for more configuration

* Remove blank layers

* Updated readme

* Improve use of EEPROM

* Credit where its due

* Use the latest iteration of rgblight code

* Keep the RGB timer running if the front LED is in RGB mode

* Fix RGB breathing animation

* Better supported RGB animation
Only thing not working is alternating, but that's not too important

* Abstract front LED handlers from main kb code

* Add support for indicator LED color changing

* Remove debug statement

* Persist indicator LED colors

* Mark custom sections in rgblight.c

* Light commenting

* Fix up keymaps

* Add/update comments

* Remove bloat from default hex

* Tidy a stray tab

* Out with the old, in with the new

* Out with the old, in with the new

* Add LAYER_STATE_8BIT for VIA keymap
kylekuj pushed a commit to kylekuj/qmk_firmware that referenced this pull request Apr 21, 2020
* Add VIA support to mxss and general cleanup

* Add support for RGB test for FLEDs

* Add LAYOUT_all to allow for more configuration

* Remove blank layers

* Updated readme

* Improve use of EEPROM

* Credit where its due

* Use the latest iteration of rgblight code

* Keep the RGB timer running if the front LED is in RGB mode

* Fix RGB breathing animation

* Better supported RGB animation
Only thing not working is alternating, but that's not too important

* Abstract front LED handlers from main kb code

* Add support for indicator LED color changing

* Remove debug statement

* Persist indicator LED colors

* Mark custom sections in rgblight.c

* Light commenting

* Fix up keymaps

* Add/update comments

* Remove bloat from default hex

* Tidy a stray tab

* Out with the old, in with the new

* Out with the old, in with the new

* Add LAYER_STATE_8BIT for VIA keymap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants