Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[driver] Added TCS3472 missing features #578

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Mar 12, 2021

Significantly improves possible performance. Got sample-rates up to 200Hz now:

Bug

  • shrinked commandBuffer[4] -> commandBuffer[3]. Until now, the driver send a placeholder on every request witch seems not to be necessary.

Features

  • Interrupt control
    • enable and reload Interrupts
    • set interrupt persistence filter
    • set interrupt thresholds for the comparator that triggers interrupts
    • getter for clear value - (Cause the interrupt-comparator is wired to clear value, it's good to inspect actual readings to setup comparator-threesholds)
  • set and enable waittime (delay between samples, conceived for energy savings)

Sry for the reformatted code... if that's a problem, i'll fix it.

@salkinium
Copy link
Member

salkinium commented Mar 12, 2021

Sry for the reformatted code... if that's a problem, i'll fix it.

  1. We use tabs (unfortunately), not spaces. That would already reduce the noise.
  2. There is a custom .clang-format config here that you can use instead of formatting manually. https://github.com/modm-io/modm/blob/develop/.clang-format Or at least you can run it once over the file you want and then change the things that you don't like (sometimes the "strict" formatting rules are not that good).

Update: There's also a tools/scripts/clang-format.sh for only updating the files you changed: https://github.com/modm-io/modm/blob/develop/tools/scripts/clang-format.sh
Alternatively there's a git clang-format origin/develop that does that same but better? https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 12, 2021

Thanks for the rich options!

I'm fine with the modm coding-style ;) so a simple IDE-plugin, applying the .clang-format from within the working-folder, did the job.

It's dropping lovely handmade vertical alignments of Register enums but i don't care. If you don't mind either, the formatting looks good now.

@TomSaw TomSaw force-pushed the improved-TCS3472 branch from 93525eb to bd3103e Compare March 12, 2021 12:47
@salkinium
Copy link
Member

The ext/dlr/scons-build-tools submodule needs to be removed in your branch, you want to remove the folder and alaos entry from .gitmodules.

@salkinium

This comment has been minimized.

@salkinium
Copy link
Member

salkinium commented Mar 12, 2021

Oh, wait, I'm an idiot. You updated both drivers. Never mind me. 🤦‍♂️🤦‍♂️

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 12, 2021

Not exactly.. i did not update the tcs3414... but if i compare the branches i see, i've fragged tcs3414.hpp 😅
Update: tcs3414.hpp is restored from upstream/develop

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 12, 2021

Oh and excuse this additional DLR submodule... next time i'll take better care

@salkinium
Copy link
Member

Ok, since I had this sensor lying around and always wanted to test it, I added an example and tested it in hardware.

I also refactored the driver some more to make it more up-to-date to our coding style and added the Data class mechanism that we typically use now. It separates the data storage and representation from the driver, which makes it easy to copy around and transmit over a network and only does the conversion work (like getNormalizedColor()) when it's necessary. I'll do the same with the TCS3414 driver tomorrow.

Could you test my changes in your hardware when you have time?

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 13, 2021

Good morning! I'm currently reading your changes and will test on my hardware soon.

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 13, 2021

Tested and works. The new 'Data mechanism' looks nice and clean.
Thanks for the adjustments, i will update my other code too.

@TomSaw TomSaw force-pushed the improved-TCS3472 branch 6 times, most recently from 6a8678d to 53d29fb Compare March 13, 2021 10:00
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll squash the commits together when you tell me you're ready (and the CI passes).

@TomSaw TomSaw force-pushed the improved-TCS3472 branch 3 times, most recently from f7cb793 to 768b98f Compare March 13, 2021 10:40
- Use separate Data class mechanism.
- Add threshold, interrupt and wait time setters to TCS3472

Co-authored-by: Niklas Hauser <[email protected]>
@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 13, 2021

I'm ready, ... but
how about lining up another example for Atmega328p / Arduino-Nano (probably without threads).
I've developed on a A-Nano so only some cleaning and decoration is required.

let me know if you think it's worth the time.

@salkinium
Copy link
Member

Yes, go for it. You can keep the protothreads, they cost next to nothing even on AVRs.

@salkinium
Copy link
Member

I'm sorry, did you mean that I or that you will add another AVR example?

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 15, 2021

This Sensor is very popular for beginners. They f.e. build candy-sorting machines of it (they work soooooooo 🐌 slow).
My thought was: that's the arduino-nano example, man let's not scare the folks and keep (at least) one example simple + blocking + polling and take the chance to give a lesson in performance:

I want to ad a second example using tcs3472s interrupt-feature, explaining: This alone frees up tons of cpu-resources.
The third example introduces protothreads, explaining the advantage of non-blocking i2c-transactions.

But maybe, that's over-ambitios 😓: no arduino-minded person will ever stumble over the lines.

Okay, i'll rest in peace with example 3 ... coming soon.

@salkinium
Copy link
Member

salkinium commented Mar 15, 2021

I also think having one example with interrupts and protothreads is best.

I don't want to demotivate you, however, our examples are regrettably not very good tutorials, often they are under-documented and at the moment we mostly (ab)use them as manual integration tests. It's something I would like to change, however, my ToDo-List is quite long and there are always more important things on it… 🙃

I would perhaps recommend doing a tutorial like this as a blog post on your homepage, since that absolves you from the "pressure" of contributing to modm and you can set the level of detail and the structure of the tutorial completely by yourself. We could then link to this in the protothread module docs.

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 15, 2021

Good recommendation. Await 2 examples later today...

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 15, 2021

So here's one example using threads and interrupt. I'm 99% sure this works but can't test now cause my one and only Nano just got fail-fused 💀 -> reanimation failed. This is defacto my facepalm-emoji-moment... but i'm to lazy to look for it 😁

Fresh Nanos have been ordered for testing.

@TomSaw TomSaw force-pushed the improved-TCS3472 branch 6 times, most recently from 10eecd4 to 548b69b Compare March 16, 2021 09:06
@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 16, 2021

Fresh head 🧠 and got my Arduino-Nano board back to life.

  • The rich example is tested and done ✅

Go, merge the stuff into develop 😁. Thanks for your guidance, and excuse this chaotic (noizy) contribution-trip. I've learned a lot and looking forward to my request.

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squashed into the previous commit and removed the while(! thing for clarity.

@salkinium
Copy link
Member

Apologies for the CI, it usually takes 20-30mins, not 5h. CircleCI seems to have significant issues, we're in contact with their support.

@salkinium
Copy link
Member

salkinium commented Mar 16, 2021

Thanks for your guidance, and excuse this chaotic (noizy) contribution-trip. I've learned a lot and looking forward to my request.

You're welcome! I'm always glad to help new contributors navigate these complex waters. There's a lot of bullshit gatekeeping in embedded, I'm not a fan of it…

@TomSaw
Copy link
Contributor Author

TomSaw commented Mar 16, 2021

True, it requires various experience-loops to finally keep repo-things horizontal. But i appreciate the requirement and helpfulness of most of the stumbling blocks.

Manually checking the result by wrapping while(! ... ) is not required 😵 the better it is 😏. I'll study the protothreads again, with new eyes.

Damn, i'm glad having contributed a little - but in my scale remarkable - piece to my favorite framework 😇

@salkinium salkinium merged commit 850b554 into modm-io:develop Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants