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

Adds Gray Studio HB85 Initial Support #5311

Merged
merged 11 commits into from
Mar 6, 2019
Merged

Adds Gray Studio HB85 Initial Support #5311

merged 11 commits into from
Mar 6, 2019

Conversation

fcoury
Copy link
Contributor

@fcoury fcoury commented Mar 5, 2019

HB85 uses ps2avrGB firmware, so used the same matrix used by the TGR Alice but added additional rows.

Description

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

@fcoury
Copy link
Contributor Author

fcoury commented Mar 5, 2019

Here's how the matrix looks on Bootmapper Client:

image

I also checked the pins manually with a multimeter and they are all apparently correct.

@fcoury fcoury marked this pull request as ready for review March 5, 2019 15:36
@mechmerlin
Copy link
Contributor

Can you put this in the gray_studio directory.

@fcoury
Copy link
Contributor Author

fcoury commented Mar 5, 2019

Can you put this in the gray_studio directory.

Of course! Making the change right now.

@fcoury
Copy link
Contributor Author

fcoury commented Mar 5, 2019

@mechmerlin this is feature complete, ready for review.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Additionally, could you remove the i2c files, and instead, use the correct functions as found in /drivers/avr/i2c_master.c?

}

void backlight_init_ports(void) {
DDRD |= (1<<0 | 1<<1 | 1<<4 | 1<<6);
Copy link
Member

Choose a reason for hiding this comment

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

Could you update these to use the GPIO Control functions, instead?

https://docs.qmk.fm/#/internals_gpio_control

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drashna can you point me in the right direction of how the GPIO functions would translate here? I will implement it on the other part myself, just need some guidance.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the actual code to handle this is here:
https://github.com/qmk/qmk_firmware/blob/master/quantum/quantum.h#L147-L169

That would be the best reference, but not the most intuitive.

A good example is one of my commits, converting everything to these:
4038308?w=1

Though, this may take a bit of testing to make sure that it's using the correct commands.

void backlight_set(uint8_t level) {
if (level == 0) {
// Turn out the lights
PORTD &= ~(1<<0 | 1<<1 | 1<<4 | 1<<6);
Copy link
Member

Choose a reason for hiding this comment

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

Could you update these to use the GPIO Control functions, instead?

https://docs.qmk.fm/#/internals_gpio_control

@@ -0,0 +1,106 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Cries in i2c


OPT_DEFS = -DDEBUG_LEVEL=0

SRC += i2c.c
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SRC += i2c.c
SRC += i2c_master.c

@fcoury
Copy link
Contributor Author

fcoury commented Mar 5, 2019

Additionally, could you remove the i2c files, and instead, use the correct functions as found in /drivers/avr/i2c_master.c?

@drashna thanks so much for the review! I reported here on this thread, but sorry for not making it more clear. If I use ic2_master it crashes on the first i2c_transmit call. Getting i2c.c back into the folder was the only way I could make this particular board work.

I am open to ideas on how to move forward.

@drashna
Copy link
Member

drashna commented Mar 6, 2019

Well, it seems that things are handled very different, But I'm guessing that you tried the method that the Alice used, but without success?

I suspect you have already. But I'm not sure.

@fcoury
Copy link
Contributor Author

fcoury commented Mar 6, 2019

@drashna yes, that was my original attempt. If you look at the commit history I started with i2c_master but it never worked.

@pelrun
Copy link
Contributor

pelrun commented Mar 6, 2019

You should have been using address 0x58 in i2c_transmit instead of 0xb0.

Edit: not actually true. Documentation needs fixing.

@fcoury
Copy link
Contributor Author

fcoury commented Mar 6, 2019

@pelrun whoa, seriously!? Thanks!

How did you find that? I even analyzed ps2avrGB source code and the underglow pin is 0xb0.

I will give it a shot when I get back home, but would you mind explaining why 0x58?

@pelrun
Copy link
Contributor

pelrun commented Mar 6, 2019

I2C slave addresses are only 7 bit. On the wire they're sent MSB first, and then followed by the read/write bit. Manually doing the transaction means passing (slave_addr << 1 | rw_flag) as the first byte. But i2c_transmit should take slave_addr directly.

@pelrun
Copy link
Contributor

pelrun commented Mar 6, 2019

...and crap, it looks like the avr version of i2c_transmit isn't doing that properly anyway. At the very least, the documentation, and the avr and arm drivers are inconsistent.

@pelrun
Copy link
Contributor

pelrun commented Mar 6, 2019

In any case, I'm waiting on #4974 to land, which fixes some issues with i2c_master on avr, so you could have been affected by that.

@fcoury
Copy link
Contributor Author

fcoury commented Mar 6, 2019

Ok that is very interesting @pelrun, thank you very much.

@drashna since this is working as is would you consider approving it and I will refactor it to use i2c_master once those fixes are in?

Let me know what you think is best.

keyboards/gray_studio/hb85/program Outdated Show resolved Hide resolved
keyboards/gray_studio/hb85/rules.mk Outdated Show resolved Hide resolved
@fcoury
Copy link
Contributor Author

fcoury commented Mar 6, 2019

@zvecr all your feedback has been addressed. I think the only thing left is using the GPIO pins functions on the two spots @drashna pointed out. I am not sure how to use them, so if you can point me in the right direction that would be appreciated as well.

Once again thank you for your review!

@drashna
Copy link
Member

drashna commented Mar 6, 2019

@drashna since this is working as is would you consider approving it and I will refactor it to use i2c_master once those fixes are in?

Yeah, that sounds good to me.

I think the only thing left is using the GPIO pins functions on the two spots @drashna pointed out. I am not sure how to use them, so if you can point me in the right direction that would be appreciated as well.

Posted above, but again, here is an example from when I did this:
4038308?w=1

The only missing command is:
writePinLow.

@fcoury
Copy link
Contributor Author

fcoury commented Mar 6, 2019

@drashna converted and tested:

hb85

@drashna
Copy link
Member

drashna commented Mar 6, 2019

Awesome, thanks!

@drashna drashna merged commit ba11a1c into qmk:master Mar 6, 2019
@fcoury fcoury deleted the hb85 branch March 6, 2019 20:56
Shinichi-Ohki added a commit to Shinichi-Ohki/qmk_firmware that referenced this pull request Mar 7, 2019
* 'master' of https://github.com/qmk/qmk_firmware: (76 commits)
  [Keyboard] Adds Gray Studio HB85 Initial Support (qmk#5311)
  [Keymap] Add KC_MAKE keycode to my userspace and keymaps (qmk#5324)
  [Keyboard] handwired/trackpoint: refactor and readme cleanup (qmk#5325)
  [Keyboard] re-arrange the pinout for backlight support (qmk#5321)
  [Keymap] update: simplify tapdance code for Minidox/keymaps/dustypomerleau (qmk#5315)
  [Keymap] Add vxid planck layout (qmk#5271)
  increase size of note counter variable to avoid overflow
  Adopted LAYOUT, added some keys (qmk#5320)
  Add more "decent" text editors (qmk#5308)
  Add DZ60 Tsangan Layout + Fn layer (qmk#5319)
  [Docs] Fix function signature (layer_state_set_*) (qmk#5313)
  Correct keyboard and layout dimensions for 40percent.club Foobar (qmk#5310)
  Dactyl Manuform 5x6, 5x7, and 6x6: QMK Configurator fixes and partial code clean-up (qmk#5307)
  Fixed default to dissable VIA (qmk#5309)
  Clean up debounce a bit (qmk#5255)
  Fix aanzee Configurator config file (qmk#5286)
  Separate keymaps to VIA enabled/dissabled. (qmk#5302)
  [Keymap] Add RGB config and controls to userspace (qmk#5299)
  Add LED Matrix to Features list for easy reference (qmk#5280)
  [Keymap] layout/community/ortho4x12/symbolic update (qmk#5274)
  ...
slugger7 pushed a commit to slugger7/qmk_firmware that referenced this pull request Mar 7, 2019
* Gray Studio HB85 Initial Support

* Fixed README image

* Updated README

* Disabled Bootmagic and Console for HB85

* Fixed Numpad 4 matrix place

* Fixes board crashing with RGB enabled

* Moved HB85 files to gray_studio folder

* Uses old i2c library since this version makes RGB underglow work

* Improved default keymap with underglow control layer

* Removes obsolete program and uses generic script instead

As per zvecr feedback

* Uses GPIO Functions to initialise and set RGB underglow PINS
chie4hao pushed a commit to chie4hao/qmk_firmware that referenced this pull request Mar 28, 2019
* Gray Studio HB85 Initial Support

* Fixed README image

* Updated README

* Disabled Bootmagic and Console for HB85

* Fixed Numpad 4 matrix place

* Fixes board crashing with RGB enabled

* Moved HB85 files to gray_studio folder

* Uses old i2c library since this version makes RGB underglow work

* Improved default keymap with underglow control layer

* Removes obsolete program and uses generic script instead

As per zvecr feedback

* Uses GPIO Functions to initialise and set RGB underglow PINS
zer09 pushed a commit to zer09/qmk_firmware that referenced this pull request Mar 31, 2019
* Gray Studio HB85 Initial Support

* Fixed README image

* Updated README

* Disabled Bootmagic and Console for HB85

* Fixed Numpad 4 matrix place

* Fixes board crashing with RGB enabled

* Moved HB85 files to gray_studio folder

* Uses old i2c library since this version makes RGB underglow work

* Improved default keymap with underglow control layer

* Removes obsolete program and uses generic script instead

As per zvecr feedback

* Uses GPIO Functions to initialise and set RGB underglow PINS
slugger7 pushed a commit to slugger7/qmk_firmware that referenced this pull request Apr 3, 2019
* Gray Studio HB85 Initial Support

* Fixed README image

* Updated README

* Disabled Bootmagic and Console for HB85

* Fixed Numpad 4 matrix place

* Fixes board crashing with RGB enabled

* Moved HB85 files to gray_studio folder

* Uses old i2c library since this version makes RGB underglow work

* Improved default keymap with underglow control layer

* Removes obsolete program and uses generic script instead

As per zvecr feedback

* Uses GPIO Functions to initialise and set RGB underglow PINS
Timbus pushed a commit to Timbus/qmk_firmware that referenced this pull request Jun 23, 2019
* Gray Studio HB85 Initial Support

* Fixed README image

* Updated README

* Disabled Bootmagic and Console for HB85

* Fixed Numpad 4 matrix place

* Fixes board crashing with RGB enabled

* Moved HB85 files to gray_studio folder

* Uses old i2c library since this version makes RGB underglow work

* Improved default keymap with underglow control layer

* Removes obsolete program and uses generic script instead

As per zvecr feedback

* Uses GPIO Functions to initialise and set RGB underglow PINS
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.

5 participants