Support for Trill capacitive sensors

[giuliomoro]

This is the "Linux" library (available here) wrapped into libDaisy. The low-level I2c I/O stuff is done here: https://github.com/giuliomoro/libDaisy/blob/64c08470c079693cd9e2ab59cf2b85a5cff4727c/src/dev/trill/I2c.h

Contributions by me and @virvel .
Tested by @dromer and @virvel when it was based on 17ee39b.
Previous discussion giuliomoro#1

Closes #602

[giuliomoro]

OK it's now brought up to date with the latest upstream and I even figured out how to do the formatting. @virvel @dromer I'd appreaciate if you have a chance to test this again.

[virvel]

OK it's now brought up to date with the latest upstream and I even figured out how to do the formatting. @virvel @dromer I'd appreaciate if you have a chance to test this again.

I can confirm that this works!

[stephenhensley]

Hi y'all! This is looking great!
Thanks for the huge contribution!

While I think there is quite a bit that would make this feel more like the rest of the libDaisy, but I'd be happy to merge it with a few minor tweaks.

At a minimum I think we'll want to:

1. The Trill object should be added to the daisy namespace instead of being global
2. I'd suggest that we also wrap most/all of the internals that have been added to support the Trill object in a daisy::trill namespace (this is to help avoid things like trill's I2c from being included in the global or daisy namespace and causing any confusion.
3. the /src/dev/trill/Trill.h file should be included in daisy.h so user's don't need to manually include the path (the examples could then be updated as well)

From a style/consistency perspective, there are a few more changes that would affect the API if they're changed later:

• class methods in libDaisy typically start with a capital letter instead of camel-case (e.g. Trill:readI2C() -> Trill::ReadI2C()
• the initialization class method in libDaisy is typically called, Init()
• typically the Init method takes a Config struct with any settings the user might provide
  • Reason for this is to provide a more stable user API while allowing drivers to add features/additional configuration without changing how the user interacts with the object.

Since this would be a new device, I'm open to the API changing a bit over time. So the bullet-list items aren't strictly necessary at this time, but would be welcome at any point.

However, I would like for the initial few points to be addressed before we merge it.

---

I don't foresee any of the recent/upcoming changes having an impact on your PR. So no rush.
I had honestly thought I left a comment similar to this several months ago, but I must not have finished writing it or something.

Thanks again for the contribution, and let me know if you any questions!

[github-actions]

Test Results

150 tests  ±0   150 ✅ ±0   0s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 156e58a. ± Comparison against base commit 9498417.

[stephenhensley]

@giuliomoro I was able to do a bit of testing with this, and noticed that my sensors are on firmware version 2, which lead to a few issues.

So in order for the Trill example to work properly, I needed to modify the setup method to not try to set the scan mode since this requires version 3.
This seems inconsistent with the rest of the setup method which does do some handling based on the firmware versions available.

After this modification, and the addition of:

LDFLAGS += -u _printf_float

to the Makefile to allow printing of floats I was able to see my Trill Bar working correctly.

If the expectation is for this to work with multiple firmware versions on the sensors (I'm not aware of how to update mine, but I haven't looked into it much yet), then we should put a conditional check for this as well.

I do also have a Trill Craft, but I haven't tested that example yet (I need to the correct jumpers in my office to get it connected to the daisy), but it looks like it does not require the floating point print linker setting mentioned above.

---

Apart from that, I think at a minimum I'd like for the three minor changes I mentioned last month to get made, and then we can merge this.

When we do merge this, I'll make a note in the corresponding release's notes (and maybe also in the Trill class docstring as well) to indicate that we'll treat this API as unstable for now so we can work gradually make it a bit more consistent with the rest of libDaisy without qualifying each tweak as a major version change.

Please let me know if you have any questions.
Even doing a small amount of testing lead to some exciting ideas for things to do with Daisy + Trill 😄

[dromer]

@giuliomoro can you have a look at the comments? would be nice to move this forward!

[giuliomoro]

@dromer I'd love to, but it's going to be a month till we ship the thing I am currently working on, before I can get back to this.

I'd really want to get this merged swiftly and not add unnecessary burdens, but I want to have a look ahead for maintainability purposes.

In general I'd really wish I could just drop the upstream files in here, with minimal wrapping modifications, rather than having to rename methods and variables to conform to a style. I appreciate that daisy strives to present a uniform-ish interface, but if that means that all changes for external libraries (such as this one, not sure if you have others or plan to) will require more effort to be backported, it may be an obstacle to keeping the library up to date.

So, wrt #657 (comment) @stephenhensley , would it be OK to only do the numbered list items and not the bullet points?

An alternative is to provide a small wrapping class (e.g.: DaisyTrill?) that complies with everything you say. It would privately include Trill.h and hold a pointer to a Trill object and expose a minimal set of methods and a Trill* getTrill() method for those interested in including Trill.h directly and accessing its more advanced functions. With such a wrapper in place, the library update process would be:

• get upstream Trill library
• pass it through clang-format
• commit

Or, even better, the most maintainable solution:

• include the upstream Trill repo as a submodule
• to update, simply update the submodule hash and commit

[dromer]

The wrapping class actually sounds like a pretty good idea as it would give the most consistent user-facing API while keeping the Trill library separate and easy to update (a submodule sounds good!)

Of course lets see what @stephenhensley has to say ;)

[stephenhensley]

@giuliomoro thanks for your reply!

I'll have to think a bit on the overall plan, but my initial thoughts are that the minimum criteria from my first comment:

1. The Trill object should be added to the daisy namespace instead of being global
2. I'd suggest that we also wrap most/all of the internals that have been added to support the Trill object in a daisy::trill namespace (this is to help avoid things like trill's I2c from being included in the global or daisy namespace and causing any confusion.
3. the /src/dev/trill/Trill.h file should be included in daisy.h so user's don't need to manually include the path (the examples could then be updated as well)

would still be things I'd consider requirements for this to be merged, with all other style-conformance/uniformity being optional.
The reason for these is primarily to prevent overlap with users' app code, and collisions within the rest of libDaisy with generic naming of things like I2c, etc.

My initial impression of this contribution was to provide (with the linux driver as a starting point) an API for accessing trill sensors that would eventually feel consistent with the rest of libDaisy, and not just a bundling the existing trill-linux sources into libDaisy (similar to how CMSIS DSP is currently included in the library).

With that said, I am open to the latter approach, whether that includes switching the directly copied files to be located in a submodule, or leaving modified copies within libDaisy (maybe more clearly in a separate third-party source directory).
However, the Bela-trill repo is not very active, and has not been updated in the last few years. So I personally don't think there is a ton of value in making upstream-syncing a priority.