Added Keebfront Vanguard65

[mrnoisytiger]

Description

Added Keebfront Vanguard65 Keyboard.

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

• N/A

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• 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).

[waffle87]

if encoders are enabled at the keyboard level, there should be some encoder functionality at the keyboard using the encoder_update_kb() function in vanguard65.c file, that way it works for QMK Configurator users.

(The QMK Configurator does not use the keymaps directory, so adding to the default keymap won't do it)
eg.

#ifdef ENCODER_ENABLE
bool encoder_update_kb(uint8_t index, bool clockwise) {
  if (!encoder_update_user(index, clockwise)) { return false; }
  if (clockwise) {
    tap_code_delay(KC_VOLU, 10);
  } else {
    tap_code_delay(KC_VOLD, 10);  
  }
  return true;
}
#endif

[mrnoisytiger]

if encoders are enabled at the keyboard level

Interesting. From my understanding though, this would put the encoder_update_kb functionality at keyboard level as well, which may be contrary to the keymap definitions.

In the alternative then, can encoders be enabled solely at the user level rather than the keyboard level? I take it, it'll easily be done by just moving ENCODER_ENABLE = yes to the rules.mk files for each keymap — and then removing it from the parent rules.mk?

On another note, I'll probably submit a PR to the docs to update to reflect this requirement then shortly. 😊 Good to know for the future.

[waffle87]

From my understanding though, this would put the encoder_update_kb functionality at keyboard level as well, which may be contrary to the keymap definitions.

Well from the snippet I shared:

if (!encoder_update_user(index, clockwise)) { return false; }

This checks for an encoder_update_user() function (which lives in the keymap directory and should return false) and stops further processing if it's present and returns the correct value.

can encoders be enabled solely at the user level rather than the keyboard level?

Sure, but if encoders are meant to be installed, why not have proper support for them in the firmware?

[mrnoisytiger]

This checks for an encoder_update_user() function (which lives in the keymap directory and should return false) and stops further processing if it's present and returns the correct value.

Gotcha, seeing that in the snippet, and makes sense. However, I don't define an encoder_update_user, but instead exclusively rely on encoder_map[] for the encoder functionality. Does the encoder_map simulate the encoder_update_user() function and therefore trigger the false condition?

If so, then I think the snippet should work as is.

Sure, but if encoders are meant to be installed, why not have proper support for them in the firmware?

Good point, you're right. It's meant to be keyboard level rather than a user option, so I guess keeping it keyboard-level makes sense.

[mrnoisytiger]

@dunk2k I think the changes to the layout names caused some issues? I'm not entirely sure what's driving these changes frankly as the layout isn't exactly a standard layout.

[dunk2k]

@dunk2k I think the changes to the layout names caused some issues? I'm not entirely sure what's driving these changes frankly as the layout isn't exactly a standard layout.

This board could be seen as a 65_(ansi|iso)_blocker(|_tsangan) QMK Community Layout with a key on the right most column missing.

[mrnoisytiger]

@dunk2k I think the changes to the layout names caused some issues? I'm not entirely sure what's driving these changes frankly as the layout isn't exactly a standard layout.

This board could be seen as a 65_(ansi|iso)_blocker(|_tsangan) QMK Community Layout with a key on the right most column missing.

Yea I agree it can be seen as that, but as far as I understand, it isn't compiling since the number of keys in the keymap expected is mismatched from those actually available... I don't believe the standard layouts build in any wiggle room for the fact that a key is missing.

[drashna]

default and via keymaps should be simple. Most of this should be removed, or moved to a manufacturer keymap, per PR checklist.

[noroadsleft]

The via keymap also doesn't compile, but there are some still-pending action items there.

[mrnoisytiger]

The via keymap also doesn't compile, but there are some still-pending action items there.

Thanks @noroadsleft appreciate the check. The VIA Keymap did indeed have an extra on each, but it've confirmed all keymaps compile on my end now.

Would you mind letting me know what the other action items are? I noted in another previous review comment (which I now can't seem to find), the extra code in the VIA keymap is pretty integral to the board's function... Otherwise, the slider would do nothing, which is a critical functional component, not a manufacturer "bell and whistle".

While I'm aware that the QMK PR Checklist suggests "No custom keycodes", I find that at odds with the practical concept of actually having the board functional in its bare extents. The slider has, as one of its 3 only functions, MIDI control, which necessarily has to rely on custom keycodes. If I remove this (and other supporting code), we're left with a slider that doesn't exactly function...

I'll, therefore, need to point everyone to a custom keymap in the README (again, counterintuitive from any user perspective - especially those GitHub-illiterate) on compile time. Again, effectively gimping the product.

I hope you understand where I'm coming from with this fact that the additional code should stay. I have already left the :default clean of any code as a compromise.

[noroadsleft]

@mrnoisytiger,

I was referring to this request from @drashna, but it seems that the marquee feature of your board doesn't work without the custom code in the via keymap. I feel like I read that somewhere in this PR, but I can't find the comment now. Is that correct?

edit: typo

[mrnoisytiger]

@mrnoisytiger,

I was referring to this request from @drashna, but it seems that the marquee feature of your board doesn't work without the custom code in the via keymap. I feel like I read that somewhere in this PR, but I can't find the comment now. Is that correct?

edit: typo

That’s exactly correct. What I believe happened was that I left the note as a “review” comment and happened to close out of the “review” whatever that means in GitHub terms. The primary and key feature just won’t work without that custom code in there.

[noroadsleft]

I'm fairly useless at C, but would it be correct to say that the feature also requires the HID communication protocol that VIA provides - I think I saw something on the listing regarding a host-side application that runs on the user's PC - and therefore can't be implemented at keyboard level without also enabling VIA at keyboard level?

[mrnoisytiger]

I'm fairly useless at C, but would it be correct to say that the feature also requires the HID communication protocol that VIA provides - I think I saw something on the listing regarding a host-side application that runs on the user's PC - and therefore can't be implemented at keyboard level without also enabling VIA at keyboard level?

Great question. I guess it’s easier to give a layout of the three functions of the fader (the marque feature):

• RGB Brightness
  • Control the RGB brightness by reading the ADC and passing it directly to the QMK RGB function (with a bit of byte math)
• Layer Shift
  • Move the slider to switch layers between 0-3. Pass a ADC reading directly to QMK helper function with a byte math
• MIDI
  • This uses QMK’s MIDI abstraction to send CC channel commands with the ADC reading
  • This is where the host/computer application comes in. There are a lot of apps that can take MIDI CC values and use it as an arbitrary control, such as monitor brightness, computer volume, track volume, etc… In an effort to avoid reinventing the wheel, I believe that relying on these host applications is the best move for more advanced control (such as Adobe Premiere, Lightroom, etc…)

While there is no barrier to implementing each function individually at the keyboard level (since they all just rely on QMK helper functions), I have no way to program switching between these. Instead, the VIA keymap creates a way to “read” a value passed by VIA as to which function is selected.

In short, the part that requires VIA is being able to change the function of the slider between the various available options. This is something I wouldn’t even know where to begin programming within QMK itself but VIA provides an easy way for users to select the function (kind of like selecting an RGB effect).

Therefore, I choose to include the code for all three slider functions into the VIA keymap. That would also make it easy to develop new functions in the future since people can directly specify the VIA-“passed” value and the corresponding function.

Does that all make sense?

[drashna]

I have no way to program switching between these. Instead, the VIA keymap creates a way to “read” a value passed by VIA as to which function is selected.

I mean, you have a variable that is saving the value and setting the mode, so the only issue is how to set the mode, and see what mode it is on.

You could use encoders and/or custom keycodes to cycle through the modes, much the way that the RGB controls do. And if you need feedback about which mode it is, rgblight layers is an option.

So it's definitely possible to do all of this without VIA.

[mrnoisytiger]

I have no way to program switching between these. Instead, the VIA keymap creates a way to “read” a value passed by VIA as to which function is selected.

I mean, you have a variable that is saving the value and setting the mode, so the only issue is how to set the mode, and see what mode it is on.

You could use encoders and/or custom keycodes to cycle through the modes, much the way that the RGB controls do. And if you need feedback about which mode it is, rgblight layers is an option.

So it's definitely possible to do all of this without VIA.

While I suppose it is, I certainly do not have the know-how to do such a thing. I am only aware of the VIA way of reading this value as it gets passed in. I’m definitely not a programmer and my knowledge really ends at hobbled together pieces of code that hopefully accomplish the project goals (which they do currently)

Further, this board is advertised to customers as relying on VIA for this and it seems like vague flashes of the LED’s (only 2 spots by the way) fail to convey actual mode. For RGB Mode you actually get visual feedback what made you’re on.

If I do “LED’s flash once”, it doesn’t really tell me much without consulting a site - at which point, I would’ve used VIA to begin with.

Not only that, I left the default (non-VIA) as clean as possible to comply with the PR Checklist.

[mrnoisytiger]

Also, just in the meantime, are there any direct action items or blockers preventing an interim merge (or to help move the process)? Would prefer to have the code merged in its current state to at least get VIA approval ready.

I would be more than happy to learn more regarding improving the software here and would be delighted to continue the conversation.

[mrnoisytiger]

Bumping this PR. Please let me know if there are any direct action items here.