Adding active, dropped, and tapped for #3

[siddacious]

This is an initial working draft to expose the requested features of the 34x. The API is based on that of the LIS3DH and honestly I'm not terribly fond of it as there are enough differences between the two that the 34x isn't able to work at it's full potential as there is a lot of useful flexibility that it provides. I'm making this PR now to see if anyone else (@microbuilder perhaps?) agrees with me that it's worth putting in the effort to change the API to something like my proposal below.

You may or may not wish to just skip to checking the code at this point, lest my comments below affect your assessment.

The API for these calls is made up of a configuration method and an attribute that checks the status of the given interrupt:
active_parameters enables calls to the active attribute
dropped_parameters enables calls to the dropped attribute
tapped_parameters enables calls to the tapped attribute

Each of the _parameters calls sets the associated bit in the INT_ENABLE register to turn on interrupts for those events. If the configured criteria is met, one of the int[1|2] pins is set to the configured active state and the associated bit in the INT_SOURCE register is set to 1. The main 'issue' with this configuration is that reading the INT_SOURCE register also clears the bits for the non-data-related interrupts that we're considering in this ticket.

This means that every check of active for example will clear the status of tapped. For this reason the _parameters calls disable all interrupts except for the one for the given call. If we stick with this API, I would update the documentation to make this clearer.

This API works (I added examples for each) however it kinda rubs me the wrong way because I'm aware of what is possible. As an alternative I had something along these lines in mind:

enable_tapped(misc params)
enable_dropped(misc params)
enable_active(misc params)
events => {"active":True, "dropped":False} where there would be keys for the enabled interrupts.
probably disable_xxx calls as well?

Additionally I haven't fully decided what to do with the int[1|2] pins (the lack of documentation reflects this). My current thought is that something like:
enabled_tapped(...., int_pin=1) would allow the user to specify which pin should be used for the given interrupt. That said, since we don't have real interrupt handlers, it seems like the pins are of minimal use. I suppose they could be used to determine in the above per-interrupt API to determine if it is worth checking INT_SOURCE, but then again there are only two pins and it seems like a bit of a hack anyways.

Also active vs. inactive is worth discussing at a later point.

Please take a look and let me know what you think. Either way the API goes, I don't think this is quite ready for merging but I wanted to get it in front of someone else.
Thanks!
_B

PPS: Once we settle on an API, I'd be happy to add the new features to the arduino driver at some point in the future

[microbuilder]

Thanks for the great PR! The current code proposal looks good to me, but I agree the API seems slightly awkward.

As you pointed out, INT1/INT2 are of limited used without the ability to have callbacks, unfortunately. Normally what you would want is to enable the ADXL interrupts and when it fires handle that with an MCU-side interrupt that is configured to fire when the GPIO pin(s) is asserted.

CP pushes us more towards polling, with the consequence that this limits the response time when something like 'free fall' is detected, but I'm not sure what to propose to improve that.

An API with something like enable_x(misc params) makes sense to me since it's clear what it's doing, and you could then poll for a variable that indicates what event(s) have been recently fired and reading the value clears it internally (to avoid having to manually clear it and keep the state check loop concise)?

I don't know if disable_x is a good choice, though, and maybe the enable_x call could simply accept a bool that is default true (=enable) and passing in false disables it? I'm not sure which approach is more CP-esque???

For enabled_tapped(...., int_pin=1), I'm not sure how useful the INT outputs are for now so I'd say just kick that ball further down the field for the moment, and make the changes based on a polling model???

[tannewt]

As you know, I'm a huge fan of properties. I'd probably have an _active boolean property such as tap_detection_active and rename active to motion so its motion_detection_active. Then I'd have another property for each parameter such as motion_threshold.

I don't like the active_parameters method because its name doesn't have a verb. You'd likely say "set" which is exactly what you do to a property.

One trick we play on the LIS3DH is to use the INT pins to read interrupt state without resetting it.

[siddacious]

Thanks @microbuilder, I think I'm firmly on the "make multiple events work at once" plan. I threw together an initial draft last night based on a combination of your input and my ideas but in testing it this morning, I started running into some somewhat strange memory allocation issues seemingly related to the number of methods. I'll have to open a paren on that one before I can go much further with the API.

@tannewt I'm all for properties but I'm in a bit of a quandary about how to make them fit here. The main issue from my perspective is that with the exception of active/motion_detection, each of the interrupts need to you setup multiple parameters in different registers and do so while interrupts are temporarily disabled. While you could have a disable/enable block or context manager or something to handle that while setting one parameter within the setter, setting one but not all of the parameters seems wrong or at least prone to unexpected behavior.

active_parameters was indeed previously set_active_parameters but when @kattni reviewed the code before the PR she pointed out the set_ methods should be properties thing, but I wasn't able to square it with the above "you gotta set a bunch of things at once" thought.

I had considered having something like:

accelerometer.motion_detection_settings = {"threshold": 13, "time": 2112, ... }

but didn't want to go down that road before starting this discussion. And now I'm not sure how to make disabling fit with that. Maybe

accelerometer.motion_detection_settings = {"enabled": False }

acc.motion_detection_active = False could also work but then I think acc.motion_detection_active=True would want to throw an error if motion_detection_settings hadn't been called, but now that I think about it I don't know if I like holding all the state to make that work.

Regarding the "use the int pins to peek" trick, that's basically what I was referring to in the tail end of my original comment but wasn't happy with the disconnect between the number of pins and the number of interrupts.

[tannewt]

Yeah, I was about to recommend named tuples as a way to make sure they are all set at once. I'm also ok with "set_" prefix if its really the best way to do it. However, its usually not.

Why must they all be set at once? Don't they have default values? Can we leave it up to the user to make sure the settings are all set before the interrupt is enabled?

[siddacious]

That's a fair point on defaults. If we default tap and threshold (why I didn't do this to start, I don't know), then we don't need to take params from the user unless they want to provide them.

I'm not familiar enough with named tuples to know what a good way to use them would be. I understand the basic idea but I'm less clean on how they would specifically fit into the API.

I think I can make the 'properties for everything' API work, so I'll give that a go.

[siddacious]

I just pushed a rebased update that allows for tracking multiple interrupts simultaneously with an updated API. I also cleaned up the code in some spots to DRY it up a bit.

@tannewt I wasn't able to get the 'properties for everything' API working in a way that made sense to me without being overly complicated, primarily due to how defaults would work and how setting multiple event parameters would work. If you feel strongly that this API doesn't work, let me know and perhaps we can schedule some time to talk about the finer points.

Otherwise I'm ready to merge this unless anyone has further feedback

[ladyada]

@microbuilder hey ktown, wanna take a look?

[tannewt]

Overall I like how this reads. What do you think about making these first two kwarg-only? That way this would be:
accelerometer.enable_tap_detection(tap=1, threshold=10)

tap may read better as tap_count as well.

[siddacious]

Agreed on both counts. I'm actually liking the idea of having all the params for the new methods be kwarg-only as I think it would make the code read better.

[tannewt]

@siddacious Yeah, I like that too. Its particularly handy for methods with multiple params of the same type.

Please mention me when you've changed it. I think we can merge even if @microbuilder hasn't finished looking at it. He can always follow up after if needed.

[microbuilder]

I did a quick HW test and this seems fine to me, though the API decisions are definitely your call!

[microbuilder]

@siddacious Hi, sorry, I've had my head down in some messy HW stuff these past two days. I can hook this back up tomorrow and have a look though, but it seems like @tannewt already have it a good once over.

[tannewt]

@siddacious Let me know when the final kw-arg only changes are made and updated in the examples. Thanks!

[siddacious]

Ok @tannewt I made the changes we talked about

[tannewt]

Great! Thank you!