Add property access for setting waveform and slot.

[caternuson]

Existing approach uses a setter function:

drv.set_waveform(effect, slot)

Change or add a property style access for this. Maybe array access for slot?

drv.waveform[slot] = effect

[ladyada]

yeah that would be more elegant!

[process1183]

I will do this.

[caternuson]

Awesome. Looks like you're up for #8 also. Please do these as separate PR's.

[process1183]

Should the waveform property replace the existing set_waveform() method, or just be added to the DRV2605 class?

[caternuson]

We may end up removing it or making it "private". But for now, just add what's needed for the new interface.

And looking at this again, maybe we just want to do:

drv[slot] = effect

[ladyada]

i like drv[slot] = effect, myself. its elegant and easy to understand.

[tannewt]

I'd suggest doing drv.sequence[slot] = effect since it can include delays as well as waveforms.

[caternuson]

Are you talking about the WAIT feature enabled by bit 7? And then using sequence as a way to indicate that the value of effect is a waveform and not a wait time?

[process1183]

OK, I've made branches for the two proposed solutions. The first implements drv.sequence[slot] = effect in the sequence_property branch. The second implements drv[slot] = effect in the class_index branch. Once we decide which route to take, I'll submit a PR from the chosen branch.

I don't have the DRV2605L board (...yet. I plan to buy one tonight), so I haven't fully tested my changes.

[tannewt]

@caternuson That is what I'm referring to. My point is that not all entries are waveforms, some are pauses. I'm picturing something like:

drv.sequence[0] = Effect(1)
drv.sequence[1] = Pause(0.4)
drv.sequence[2] = Effect(10)

I'm not exactly sure what Effect and Pause would be. They could be functions but then they would read back as ints. If they were objects then the sequence would be understandable when printed back.

@process1183 I don't think your sequence_property code will allow for waits in the current way its written.

[caternuson]

@tannewt I was thinking of something similar. Since it's just a matter of setting the 7th bit, it could be done with:

# to set slot 0 to effect 1
drv.sequence[0] = 1
# to set slot 1 to a wait 0.4 secs (actual delay = 10ms * bits[6:0])
drv.sequence[1] = 0x80 | int(0.4 * 100)

So your Effect and Pause could be module level helper funcs to take care of that bit-foo and math and range check.

def Pause(secs):
    if not 0 <= secs <= 1.27:
        raise ValueError("Out of range.")
    return 0x80 | int(secs *100)

Guess Effect would just be a range check? Not sure what else it would do. Or maybe just don't even have Effect?

[process1183]

@tannewt you're right, my sequence_property code won't allow pauses currently. Out of curiosity, I looked at the current set_waveform(), and it doesn't allow pauses either. Should a new issue be opened for pauses in set_waveform()?

I like the idea of drv.sequence[slotnum] only accepting an Effect or Pause class. This would be easy to use and understand, plus it would fix my sequence_property not allowing pauses.

@tannewt, @caternuson, and @ladyada: Are you good on moving forward with the drv.sequence[slotnum] style?

[caternuson]

I'm fine with deferring adding pauses to another PR and going with @process1183 's sequence_property branch.

[tannewt]

I'm ok doing a follow up PR to support pauses but don't want it to be released as is. Otherwise, people will write code to an API that is going to change.

[caternuson]

I think the future change could be done without breaking if we just don't do anything special for setting an effect. So now and in the future it would be drv.sequence[slot] = effect. And effect is just a number like is currently done. Then the future mod would provide something like drv.sequence[slot] = Pause(0.4). Whatever Pause is would need to be added and the range checking in __set_item__ would need to change. But drv.sequence[slot] = effect could still to be made to work as it did.

[process1183]

What I was thinking was a separate Issue/PR for adding pause support to the existing set_waveform() method. I would add both the Effect and Pause classes to the sequence_property branch, then change the sequence property so that it only takes the Effect or Pause class. That way the sequence property is complete before making a PR for this issue.

[tannewt]

@process1183 I'd rather go straight to the property. I'd deprecate set_waveform in favor of it.

Anyone know if there are good names for the effects? It'd be neat to name them if not.

[caternuson]

They have names (see table in datasheet). But not what I would call good names in terms of being easy to remember and use - "Transition Ramp Down Long Sharp 2 - 50% to 0%", for example.

[tannewt]

Ah! Maybe having a separate module to define those would be helpful or at least a link in the docs. Building those names in will take a lot of memory.

[caternuson]

It's linked in the Learn Guide. You're saying also add to RTD docs?

Can the separate module (or whatever waveform names helper mechanism) be deferred to another PR?

@process1183 Do you want to go ahead and update your PR as you were suggesting here? This sounds like how we want to go with this.

I would add both the Effect and Pause classes to the sequence_property branch, then change the sequence property so that it only takes the Effect or Pause class.

[process1183]

@caternuson sure! I'll get that updated and open a PR soon.

[tannewt]

@process1183 no need for a new PR. Just push to the branch for this one and let us know it needs another look. Thanks!

@caternuson Yeah, we can wait on the helper names. It just makes it more readable.

[process1183]

Hi @tannewt, @caternuson, and @ladyada! I've added the Pause and Effect classes and changed the sequence property slots so they only accept one of them ( https://github.com/process1183/Adafruit_CircuitPython_DRV2605/commit/87e03c561ff5fc55558d7f8725e6959c1bf0c982 ). The new behavior works as expected using a Feather M0 Express (CircuitPython 3.0.2) and the Adafruit DRV2605L Haptic Motor Controller + Vibrating Mini Motor Disc.

There is one thing left that I don't know how you want to proceed with. In the Effect class, I have an id property so that getting an effect ID is intuitive: effect_id = drv.sequence[0].id. However, pylint does not like the 2 character property.

josh@dev:~/Adafruit_CircuitPython_DRV2605$ pylint adafruit_drv2605.py
Using config file /home/josh/Adafruit_CircuitPython_DRV2605/.pylintrc
************* Module adafruit_drv2605
C:242, 8: Attribute name "id" doesn't conform to '(([a-z][a-z0-9_]{2,30})|(_[a-z0-9_]*))$' pattern (invalid-name)
C:250, 4: Attribute name "id" doesn't conform to '(([a-z][a-z0-9_]{2,30})|(_[a-z0-9_]*))$' pattern (invalid-name)
C:255, 4: Attribute name "id" doesn't conform to '(([a-z][a-z0-9_]{2,30})|(_[a-z0-9_]*))$' pattern (invalid-name)

------------------------------------------------------------------
Your code has been rated at 9.82/10 (previous run: 9.82/10, +0.00)

Should I add pylint ignores for this, or use a different property name for Effect.id?

[caternuson]

@process1183 looking good.

You can add a # pylint: disable=invalid-name for that. We just try to respect the linting as much as possible, but don't need to force everything to comply. Having the variable named id seems fine.

Can you modify the Pause class so it works with seconds, instead of 10s-of-milliseconds. Do the math internally. That way the user can just think in terms of actual seconds.

# to set slot 1 to a wait 0.4 secs
drv.sequence[1] = Pause(0.4)

Change the doc strings for the properties to read like variables instead of functions. For example, change this:

        """Return the pause duration."""

to something like:

        """The pause duration in seconds."""

You only need to have doc strings for the @property.

[process1183]

@caternuson, thanks for the feedback! Having the Pause class use seconds is much better. I'll make the changes you mentioned and let you know when I'm done.

[process1183]

@caternuson, The Pause class now uses seconds, the docstrings have been adjusted, and Pylint will ignore the Effect.id property.

[caternuson]

@process1183 Cloned your latest and played around with it. I'd say it's time to PR! Specifically, from the sequence_property branch. We'll deal with any additional refinements via code review there.

thanks!

[process1183]

PR opened: #11

[caternuson]

And merged! Thanks for the updated.