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.

Sign up for GitHub

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

Jump to bottom

Noeeprom functions for rgb_matrix #9487

Merged
merged 4 commits into from
Aug 7, 2020
Merged

Noeeprom functions for rgb_matrix #9487

merged 4 commits into from
Aug 7, 2020

Conversation

tynanbe
Copy link
Contributor

@tynanbe tynanbe commented Jun 20, 2020 •
edited
Loading

Description

Added eeprom_helpers for toggle, mode, sethsv, speed, similar to rgblight versions.
Added set_speed function.
Added helper functions, similar to those in rgblight, in order to:
Add noeeprom versions of toggle, step, hue, sat, val, and speed.
Minor: spelling correction for EEPROM in a debug message.

Types of Changes

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

Issues Fixed or Closed by This PR

  • Unsure, probably should have opened one first.

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 zvecr changed the base branch from master to develop June 21, 2020 20:01
zvecr
zvecr requested changes Jun 21, 2020
View reviewed changes
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

  • clang-format should be run across the 2 changed files
  • docs/feature_rgb_matrix.md should be updated with the new functions
    • do the *_eeprom_helper functions need to be exposed as part of the public API?

@zvecr zvecr requested a review from a team June 21, 2020 22:42
@tynanbe
Copy link
Contributor Author

tynanbe commented Jun 22, 2020 •
edited
Loading

  • Ran qmk cformat
  • Updated docs/feature_rgb_matrix.md
  • Unsure if *_eeprom_helper functions need to be exposed; tried to follow the conventions of quantum/rgblight.h, but now I see it only exposes rgblight_sethsv_eeprom_helper and rgblight_mode_eeprom_helper

tynanbe added a commit to tynanbe/qmk_firmware that referenced this pull request Jun 25, 2020
@tynanbe
Note what should be removed when QMK merges PR qmk#9487
b0ed49a
Erovia
Erovia approved these changes Jun 27, 2020
View reviewed changes
Copy link
Member

@Erovia Erovia left a comment

Choose a reason for hiding this comment

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

__attribute__((weak))

@Erovia Erovia requested review from zvecr and a team June 27, 2020 19:58
@drashna
Copy link
Member

drashna commented Jul 5, 2020

I would say that the helper functions don't need to be exposed, at all.

@Erovia Erovia requested a review from a team July 7, 2020 21:15
@tynanbe tynanbe force-pushed the develop-rgb_matrix-noeeprom branch from 07f36a9 to c3d6bd4 Compare July 29, 2020 19:30
@tynanbe
Copy link
Contributor Author

tynanbe commented Jul 29, 2020

Rebased on current develop branch.
Removed rgb_matrix_*_eeprom_helper functions from quantum/rgb_matrix.h

@tynanbe tynanbe requested review from drashna and removed request for a team July 29, 2020 19:54
@drashna
Copy link
Member

drashna commented Aug 4, 2020

It looks like it grabbed a bunch of other commits too. :(

Probably needs to be rebased again. That, or some git magic.

@noroadsleft
Copy link
Member

Looks like develop was merged into this branch at some point, instead of this branch being rebased upon develop. (They're not the same thing.)

@tynanbe, you need to run git rebase -i develop, which should give you a window that looks something like this:

image

Type :1,23d and hit the Enter key. This will delete the first 23 lines of the file. Then, type :wq and hit Enter again to save and close. Git will do some magic, and then finish with a message that reads something like Successfully rebased and updated refs/heads/develop-rgb_matrix-noeeprom.

After that, run git push --force-with-lease – this will update this branch to have only the four commits that you authored.

@tynanbe tynanbe force-pushed the develop-rgb_matrix-noeeprom branch from 238d930 to 10d504c Compare August 5, 2020 12:34
@tynanbe
Copy link
Contributor Author

tynanbe commented Aug 6, 2020

@zvecr @noroadsleft Please do a final review; all requested changes have been made.

noroadsleft
noroadsleft approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

Slight __attribute__ ((weak)) ✔️ because I don't have an RGB Matrix board, but the code looks fine to me.

@noroadsleft
Copy link
Member

@noroadsleft Please do a final review; all requested changes have been made.

I actually fully reviewed this last night... then forgot to mark it reviewed 🤦‍♂️

@noroadsleft noroadsleft merged commit d44b5aa into qmk:develop Aug 7, 2020
@noroadsleft
Copy link
Member

Thanks!

@tynanbe tynanbe deleted the develop-rgb_matrix-noeeprom branch August 8, 2020 20:10
noroadsleft pushed a commit that referenced this pull request Aug 11, 2020
@tynanbe @noroadsleft
Noeeprom functions for rgb_matrix (#9487)
c4d3b24 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
noroadsleft pushed a commit that referenced this pull request Aug 27, 2020
@tynanbe @noroadsleft
Noeeprom functions for rgb_matrix (#9487)
db1713d 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
noroadsleft pushed a commit that referenced this pull request Aug 29, 2020
@tynanbe @noroadsleft
Noeeprom functions for rgb_matrix (#9487)
e34eca3 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
nicocesar pushed a commit to nicocesar/qmk_firmware that referenced this pull request Sep 6, 2020
@tynanbe @nicocesar
Noeeprom functions for rgb_matrix (qmk#9487)
ed76995 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Sep 30, 2020
@tynanbe @drashna
Noeeprom functions for rgb_matrix (qmk#9487)
eb056ac 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
kjganz pushed a commit to kjganz/qmk_firmware that referenced this pull request Oct 28, 2020
@tynanbe @noroadsleft
Noeeprom functions for rgb_matrix (qmk#9487)
c6141a0 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
@tynanbe @noroadsleft
Noeeprom functions for rgb_matrix (qmk#9487)
5a59ccb 
* Add eeprom_helpers for toggle, mode, sethsv, speed; add set_speed; add noeeprom versions of toggle, step, hue, sat, val, and speed

* qmk cformat rgb_matrix

* Add rgb_matrix_set_speed and *_noeeprom functions

* Do not expose rgb_matrix_*_eeprom_helper functions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noroadsleft noroadsleft noroadsleft approved these changes

@Erovia Erovia Erovia approved these changes

@drashna drashna drashna approved these changes

@zvecr zvecr Awaiting requested review from zvecr

Assignees
No one assigned
Labels
core enhancement needs doc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tynanbe @drashna @noroadsleft @Erovia @zvecr