Add documentation for the Potentiometer module included with KMK

[NiceManiac]

KMK includes a potentiometer module, potentiometer.py, but this does not have any documentation. i have written up some basic documentation for this module after some digging and reverse engineering, to help people who are not as persistent as me.

[xs5871]

First off: thank you so much for taking the time to add to the documentation. That's one of our weak points, and it's very appreciated.

This is where I see a problem:
The potentiometer code is kind of old, a bit weird, should really have a different name (and cover all continuous / ADC peripherals); and the examples in your documentation use interfaces that are likely to be deprecated soonish.
I don't want to just throw away your work, not only because it's obviously covering some of our blind spots, but also because some of that example code may have a place in a hypothetical "proper" analog module. I also don't want to merge something that I know I have to completely rewrite at some point.

If you're up for a dive and want to help me improve our handling of analog peripherals, that'd be awesome.
If not, we can discuss just the doc contribution and get a bit polished and somewhat more future proof.

[NiceManiac]

The potentiometer module is a bit weird, but it does work at least for now for analog signals, even if it relies on deprecated code to make it run.
I do agree that the module probably should be renamed, maybe to analog or something similar as it does apply to any analog signal coming thru the ADC ports.
I mainly just wanted to write down my findings in case anyone else was interested in getting potentiometers to work with KMK, although i am still trying to work out a good way to add a controller HID device to be able to also output analog data as controller joysticks to the pc.
As for improving the handling of analog signals i will have to do some reading on how circuitpython handles analog signals, which at the moment i don't really have time to get fully educated on, but in the meantime i think that we should be able to get the doc contribution into a useful state. as i have stated i havent analyzed the entire codebase so my writeup only included what i have managed to figure out from my evening hairpulling while trying to get my board to do what i wanted. Ofc if there is any newer features that might improve the way the module works i would be really interested in learning more about how KMK works, and also improving the documentation.

[xs5871]

That sounds great. Give me a couple of days to give this a proper look at. We may have to change the pot module a tiny bit to get to an MVP.

[piman13]

Is this gonna get merged at some point?
I missed this when trying to build stuff for a project of mine and it would have greatly helped
even wrote my own to merge before I found this

[xs5871]

It seems the original author has abandoned this PR.
There are pending change requests. Unless those get resolved the PR will not get merged.

[NiceManiac]

I never got an update when you said you were gonna take a few days to look at it. Not sure if you made any changes to the pot module, if not it should probably be fine, but i would have to update my repo to double check.

[xs5871]

We can't advise to do any of that.
For one, keyboard may not be defined where a user defines these handlers. Second, the note to deprecation is nice, but add_key and remove_key don't handle all keys correctly and should really be avoided in any code going forward.

Suggestion: we get rid of the awkward (pin, callback, *)-tuple interface and replace it with Potentiometer instances proper, and add a user-definable on_move method:

class PotAB(Potentiometer):
    def on_move(self, keyboard, module, position, direction):
        if ...:
            keyboard.resume_process_key(module, key=KC.A, is_pressed=True)
        elif ...:
            keyboard.resume_process_key(module, key=KC.B, is_pressed=False)
        # ... etc.

potentiometer = PotentiometerHandler(
    potentiometer = (PotAB(board.A1, inverted=False),)
)

Also, your example would trigger for every tiny movement, again and again, regardless if it passes thresholds between "key presses/releases". That might not be noticable with the example keys, but that is not true for every key and likely not the intended behavior.

[NiceManiac]

i am going to rewrite this part and do some testing with my joystick board, but i believe that this should be pretty easy to accomplish. The initial example is just whatever i could come up with on the fly that would work for my use case, but as the documentation was a bit lacking and my understanding of python and circuitpython is still developing i just offered up whatever i figured out that would work.
i am very happy to learn the proper ways to do things and will work on a revision with better examples as my knowledge expands.
I hope it is not bothering you too much to help with these things as i originally started learning python specifically to develop for circuitpython, and there is some quirks, so having someone knowledgable to help my understanding is great.

[xs5871]

That's alright. I'd say the documentation should only contain information about the usage and one or two very basic examples. It's not the right place to have a wall of code.

[xs5871]

I'm sorry, but that is not going to be an example in the official documentation.
There's so much wrong with this:

• The hid_report should never ever be interfaced with user code.
• And that's not even a good way to use it. You're throwing away any keys that are currently pressed.
• This does not take into account that there's no feedback about the actual volume.
  This is garantueed to generate unecessary bug reports and user frustration.

[NiceManiac]

i am not exactly sure why the hid_report was in there as this is a copied example from ZFR_KBD's RP2.65-F keymap that is included with the KMK repo. I wil have to look into how circuitpython can interface with the os if not by utilizing the hid_report. I am not that well versed in exactly this field so if you or someone else has a better example for this that would be great.

[xs5871]

I don't think we should put any kind of jank volume control examples into the "official" documentation, purely because of the third bullet point. The same "level" of volume on your keyboard will always produce different levels of volume on your computer. Terrible user experience.

[xs5871]

The recommended pattern is to first assign the RGB module to a global reference, then use that reference, not look it up by type.

[xs5871]

I never got an update when you said you were gonna take a few days to look at it.

Thank you github... There was a period when github would register my reviewes and comments, but not post them.

[NiceManiac]

I never got an update when you said you were gonna take a few days to look at it.

Thank you github... There was a period when github would register my reviewes and comments, but not post them.

Just github doing github things i guess, it is all good.

[piman13]

Not sure if this helps but as far as implementation and interaction with it. I got it to just run my custom extension to sent the values as midi commands. I'm not sure if this a "Proper" way to use this or how to interact with modules in kmk in general but it works

mind you as far as I know there are very few keyboard commands that can take a int in any form (hence using midi)

https://github.com/piman13/kmk_firmware/blob/main/docs/en/potentiometer.md

pot2midi.py

import adafruit_midi
import usb_midi
from adafruit_midi.control_change import ControlChange
from adafruit_midi.note_off import NoteOff
from adafruit_midi.note_on import NoteOn
from adafruit_midi.pitch_bend import PitchBend
from adafruit_midi.program_change import ProgramChange
from adafruit_midi.start import Start
from adafruit_midi.stop import Stop

from kmk.extensions import Extension
from kmk.keys import KC

def SendMidi(self, potnum, potarray):
    #print(potnum)
    #print(potarray)
    #potval = potarray[potnum].get_pos()
    #print("p" + str(potnum) + ": " + str(potval))

    potpositionraw = (potarray[potnum].get_state()).position
    potdirection = (potarray[potnum].get_state()).direction
    #print("potpos base: " + str(potpositionraw))
    #print("potdir base: " + str(potdirection))


    #if potpositionraw < 1 and potpositionraw !=0: print("potpos + 127: " + str(potpositionraw + 127))
    #elif potpositionraw == 0 and potdirection == 1: print("potpos D + 127: " + str(potpositionraw + 127))
    #else: print("potpos normal: " + str(potpositionraw))


    if potpositionraw < 1 and potpositionraw !=0: potposition = potpositionraw + 127 #print("potpos + 127: " + str(potposition + 127))
    elif potpositionraw == 0 and potdirection == 1: potposition = potpositionraw + 127 #print("potpos D + 127: " + str(potposition + 127))
    else: potposition = potpositionraw#print("potpos normal: " + str(potposition))

    print("p" + str(potnum) + ": " + str(potposition))
    Message = ControlChange(7,potposition)
    self.midi.send(Message,potnum)




class Pot2Midi(Extension):
    def __init__(self):
        try:
            self.midi = adafruit_midi.MIDI(midi_out=usb_midi.ports[1], out_channel=0)
        except IndexError:
            self.midi = None
            # if debug_enabled:
            print('No midi device found.')

    def Handler(self, potnum, potarray):
        return lambda FunctionReturn : SendMidi(self, potnum, potarray)
        
    def on_runtime_enable(self, sandbox):
        return None

    def on_runtime_disable(self, sandbox):
        return None

    def during_bootup(self, sandbox):
        return None

    def before_matrix_scan(self, sandbox):
        return None

    def after_matrix_scan(self, sandbox):
        return None

    def before_hid_send(self, sandbox):
        return None

    def after_hid_send(self, sandbox):
        return None

    def on_powersave_enable(self, sandbox):
        return None

    def on_powersave_disable(self, sandbox):
        return None

[xs5871]

It's been a year since I looked at the Potentiometer code. I don't think there's a way around refactoring, or even completely reimplementing. It is basically unusable without creating jerry-rigged jankness around it to tie it into KMK.

[NiceManiac]

It's been a year since I looked at the Potentiometer code. I don't think there's a way around refactoring, or even completely reimplementing. It is basically unusable without creating jerry-rigged jankness around it to tie it into KMK.

i agree, but i do believe we should be able to get some kind of "soon to be deprecated" documentation for it, if not just the very basics until a reimplementation is complete. just in case someone wants to experiment with a jerry rigged solution to get their keyboard to work until the new implementation is complete

[xs5871]

but i do believe we should be able to get some kind of "soon to be deprecated" documentation for it, if not just the very basics until a reimplementation is complete. just in case someone wants to experiment with a jerry rigged solution to get their keyboard to work until the new implementation is complete

I disagree. A feature that is so broken that it requires bad code to get it working should be removed, not documented.
The documentation is seen as (or at least should be) a guide of best practice and common recommendations.
That is especially true for a project that attracts people with widely varying background in programming fundamentals.
Incentivizing questionable coding habits will at the end increase the amount of debugging and user support we have to deal with.

[xs5871]

btw, there's #1054 if you want to check it out.

[NiceManiac]

but i do believe we should be able to get some kind of "soon to be deprecated" documentation for it, if not just the very basics until a reimplementation is complete. just in case someone wants to experiment with a jerry rigged solution to get their keyboard to work until the new implementation is complete

I disagree. A feature that is so broken that it requires bad code to get it working should be removed, not documented. The documentation is seen as (or at least should be) a guide of best practice and common recommendations. That is especially true for a project that attracts people with widely varying background in programming fundamentals. Incentivizing questionable coding habits will at the end increase the amount of debugging and user support we have to deal with.

This is fair enough. The feature might be broken but it has the basics working. i was just talking from the perspective of a user that wanted a feature to work and managed to get it working. i do not disagree that it needs a rewrite and i am more than happy to help out with such a task, although i do not know how much help i would be because of my somewhat limited knowledge of the complete project codebase and lacking knowledge of advanced usage ov circuitpython.

[xs5871]

You actually can help: By testing and giving feedback on the new analog input module draft PR I linked above.