add values property and refactor other properties

[jfurcean]

This exposes the read_data() to users so it can operate like most of the Arduino versions of this library (e.g. WiiChuck). They can call the read_data() function at the top of their loop to pull the data and then use the properties throughout to retrieve the data. The properties themselves no longer call read_data(). Likewise, this offers a new property values that reads the data once and passes back a named tuple of all of the other properties (#21). The named tuple uses the naming conventions of the data diagram from WiiBrew. I am open to other suggestions, though I would like to be consistent because there are several other extension controllers I want to base off of this library.

[caternuson]

We can effectively do the same thing via a values property. Instead of exposing _read_data, it would get called in values.

Simple code that doesn't need super fast speed can be written like the current examples:

if nc.button_C or nc.button_Z:
    print("button")

No need to make any special calls to read the data. Yes, this would take two I2C transactions for each button read. But for most use cases, hopefully that doesn't matter. (esp. with recent I2C improvements)

For use cases where the additional reads slow things down, can do:

values = nc.values
if values.buttons.C or values.buttons.Z:
    print("button")

That would be a single I2C transaction. This approach also helps makes it a little clearer that in this second case buffered values are being used, not the actual current values from the Nunchuk.

I've played around with this approach and have pushed some code here:
https://github.com/caternuson/Adafruit_CircuitPython_Nunchuk/blob/iss21/adafruit_nunchuk.py

[jfurcean]

I like this approach. The values is even clearer with the nested named tuples. I think it will translate well to the other extension controllers.

[jfurcean]

I tested your above approach and it is a lot slower when you use this it on the classic controller that has 20 inputs. If you are reading from one joystick not only are you doing an ic2 read that input, but you are parsing the entire data packet for all the other inputs even though you are not using them. Also, I had to convert my file to an .mpy in order to run because of memory issues.

[caternuson]

Hmmm. Yah, I was assuming / hoping the data unpacking was fast enough it didn't need to be optimized. But can look at refactoring that better.

[jfurcean]

Can you take a look at commit 97aad9e? It allows for three options.

1. The values property approach.
2. The slower code like the examples where each property call completes a I2C transaction.
3. A combination of the read_data and the property values approach but only one I2C transaction is necessary.

nc = adafruit_nunchuk.Nunchuk(board.I2C())

values = nc.values
if values.buttons.C or values.buttons.Z:
    print("button")

nc = adafruit_nunchuk.Nunchuk(board.I2C())

# each one will do a separate I2C transaction
if nc.button_C or nc.button_Z:
    print("button")

The new force_read argument helps handle this. This would also allow for individuals to easily port Arduino projects.

nc = adafruit_nunchuk.Nunchuk(board.I2C(),force_read=False)

# completes the I2C transaction
nc.read_data()

# parses the button states from the last call to nc.read_data()
if nc.button_C or nc.button_Z:
    print("button")

[caternuson]

I think for porting Arduino projects, they can just use the values approach, which is effectively the same thing. If that means more lines of code need changing, that's OK. So no need to support scenario 3 above.

For optimizing the reads, I think this same general flag for doing the read and updating the buffer is good. Sort of like what you have in place with force_read. But we can manage it automatically behind the scenes. I've pushed some changes here as an example (but also I'm still thinking about all this):
https://github.com/caternuson/Adafruit_CircuitPython_Nunchuk/blob/iss21/adafruit_nunchuk.py

Hopefully this refactoring of the byte processing helps with the issue for when there are a ton of parameters? With the Nunchuk, it's hard to tell.

Just to be clear on the .mpy file creation - was that for the Classic Controller code you wrote? Or was it for this Nunchuk code? In the long wrong, everything becomes an .mpy. For the bundle, all libraries are converted to .mpy automatically as part of the bundle creation.

[jfurcean]

I am OK with the processing of the do_read and removing support for scenario 3. What is the benefit of doing the byte processing for all the buttons in one function instead of in the individual ones? It isn't a big deal for this particular library, but it is really hard to understand what exactly is going on. Where the original with the byte processing in each individual property is much easier to understand what is going on. I will push another update with using your do_read approach with how I think the byte processing should happen.

The .mpy comment was just an observation and it was for the Classic Controller code which has many more properties.

[jfurcean]

I did some playing around with your approach on the Classic Controller and it looks like it will work. Do you want to me to close this pull request?

[caternuson]

What is the benefit of doing the byte processing for all the buttons in one function instead of in the individual ones?

None really. Just didn't want to have a guzzilion _foo() functions for each property, each with its related byte processing code. That was my one attempt at trying to be clever and consolidate things a little.

I'm wondering if we could consolidate all those into a single private function, something like a _process_bytes(), instead of _joystick, _buttons, etc. And pass in some parameter(s) to control logic in there that only parses what's needed.

I will push another update with using your do_read approach with how I think the byte processing should happen.

Sure, if you want to. But if this is holding you up and you want to move on, I can PR (eventually) what I've been hacking on.

[jfurcean]

After looking it over, I am okay with the _buttons, _joystick, _accelleration. I think there needs to be comments to identify what is being parsed out though. This is what I have for the Classic Controller:

    def _buttons(self, do_read=True):
        if do_read:
            self._read_data()
        return self._Buttons(
            not bool(self._read_data()[5] & 0x10),  # A
            not bool(self._read_data()[5] & 0x40),  # B
            not bool(self._read_data()[5] & 0x8),   # X
            not bool(self._read_data()[5] & 0x20),  # Y
            not bool(self._read_data()[4] & 0x2),   # R
            not bool(self._read_data()[4] & 0x20),  # L
            not bool(self._read_data()[5] & 0x4),   # ZR
            not bool(self._read_data()[5] & 0x80),  # ZL
            not bool(self._read_data()[4] & 0x4),   # start
            not bool(self._read_data()[4] & 0x10),  # select
            not bool(self._read_data()[4] & 0x10),  # home
            )

[caternuson]

Do you also have individual properties for each button? Something equivalent to button_Z and button_C in this Nunchuk controller?

[jfurcean]

I do currently, but I think I am going to remove them. I just made a push to this one with your changes and removing the individual properties. What do you think about this one?

[caternuson]

That'd work. I actually did that also at one point. Only issue is that it breaks the current API, since button_Z and button_C go away. Code would need to be changed to buttons.Z and buttons.C. We could break the API if we wanted though. I guess I was trying to keep backwards compatibility, but without any huge justification for it.

Are you finding that with the Classic Controller all that parsing is slowing things down if you just want to read an individual button?

[jfurcean]

You could keep those two properties for the adafruit_nunchuck if you didn't want to break backwards compatibility, but I don't think I would want to implement individual properties in the other extension controllers. Maybe remove the individual properties from the examples and push people to either use values, buttons, joystick, etc. I am not experiencing issues with the Classic Controller if I don't use the individual properties.

[caternuson]

Thanks for continuing to work this. I think it's getting very close to being merged. See the few comments I've added.

Also - it looks like the nunchuk_mouse.py example got removed?

[jfurcean]

I removed the nunchuk_mouse.py because it didn't properly handle button presses. It may have worked previously because of the slow i2c transactions. I didn't see what the benefit of having it included while there was the nunchuk_analog_mouse.py example, but I can fix the issues and add it back.

[caternuson]

OK, that makes sense. Yah, it seemed like nunchuk_analog_mouse.py was sort of an updated version of nunchuk_mouse.py anyway. I'm OK with that then. I like having it be just two examples - one for joystick and one for accelo.

[caternuson]

One last tweak.

[caternuson]

Even here, let's remove this and just access directly in the conditionals.

[caternuson]

Change to use nc.buttons.Z.

[caternuson]

Change to use nc.buttons.C.

[caternuson]

Awesome. Thanks!