-
Notifications
You must be signed in to change notification settings - Fork 585
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
Add TM1650 LED controller support #1855
Conversation
Thanks for the PR. Looking at the code, it seems almost the same as the TM1637: https://github.com/dotnet/iot/tree/main/src/devices/Tm1637 I guess it would make sense to merge your work into the TM1637. We can rename the folder to Tm16xx and have both binding at the same place. Looking quickly at the code, build a base one with all the common elements should be the way to go. Happy to hear about your thoughts here. |
Yes. It's similar but the bit sequence is reversed and the ack feedback is different. Some code, like the Character.cs, is same. But the low level code is different. Maybe, actually it's better in some case, to merge these 2 controller. But by replacing the existed one to a new one, may break user code. That's why I create this library separately. |
There are ways to keep compatibility. If you have heritage and keep the high level functions the same, you'll be all good for example.
We should then not copy/paste but rather reuse and that's one more reason to try to have both merged. I understand that there may be a break in the namespace for example but it would take 30 seconds to fix. |
I could do that merging by: Renaming the namespace and the class name. |
I would create a base class so all common elements can get there. Then keep the Tm1637 and add the Tm1650, both deriving from the base class. Each of them can implement what is specific and different. So you still have 2 different classes, very little break for the existing one. And all up, having 2 classes makes it easier. |
Good point. I'll do that in coming days. Another PR will be opened after testing. But the Tm1637 is used as the namespace, not only the class name. |
You can use this one if you want. No need for a new PR. And thanks for the discussion and the efforts! |
Still testing TM1637. The new code cannot display segment correctly on my TM1637 testing chip. |
Hi Ellerbach, Code are written and tested. Please continue. BTW, the conflicting, which is about ArgumentException, is processed as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've made a quick review and I think you can overly simplify things here. In short:
- no need of the interfaces
- move everything you can in the base class with internal for the visibility of what the upper functions may need
- rename some of the private functions to the same name so it can be used in the base class
- You may keep the DataCommand with a simple switch with the type of Model to call the correct one. Private constants can work as well, the switch pattern still will be needed
- Don't forget to move back the samples once all this is done ;-)
src/devices/TitanmecLed/Tm1637.cs
Outdated
|
||
#region Low level IO | ||
|
||
private protected override IEnumerable<bool> ByteToBitConverter(byte data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything that is the same should move to the base class and be internal for the access to the upper classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each device has its own ByteToBitConverter body. For example, TM1637 use LSB first mode but TM1650 use MSB first instead. So only an abstract method is placed in the base class.
Samples added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please only do minimal changes to the code to add support:
- rename folder and class name to Tm16xx and make it an abstract base class with common functionality
- add two derived classes Tm1650 and Tm1637 which only implemented protected abstract/virtual members for stuff which differs between bindings and have constructors which mostly pass everything to base constructor
1 Rename to what? TM1650AndTM1637AndOtherStuffsFromTheSameCompany? All shared code are already in the abstract class. |
If you support only two bindings: then name should be: If you add support for more devices and naming will need to be adjusted then we think about it in next PR when you add it. |
Good point. Thanks. TM16xx will be used instead. Checkin in coming days. |
The checkin c91c6dd has only namespace changed, since it contains multiple deletion and creation, lol. |
All issues other than removing interfaces are resolved and checked in. About interfaces: For this driver, if user want to access one driver specifically, use the class name directly, no matter those interfaces exist or not. But if the user need to work with multiple kinds of device, the interface will help to keep the main code clean. Or, a layer of "wrapper classes" need to be coded, passing the method accessing with the same signature, maybe from interface or abstract class. TM16xx is a big family with many kinds of devices. Some devices support some features but others do not. Some use I2C protocol while some others use serial. Some devices use 16-bit segment to support English alphabet display, accepting both 2-byte-character (which is not included in this PR yet) and 1-byte-character (conversion code required) encoding. |
@scegg while I do agree that in some cases interfaces can bring a great value, in this case, I feel it does complexity the binding. A not supported exception would be fine for the cases where something won't be supported. Or just doing nothing. We have similar cases in some other sensor family. I would as well move as much code as possible in the base class. You pass the type, so in the bit conversion function, depending on the device type, you can branch one or the other conversion. And when another will come, same thing. It does avoid having too much code all over the place. While we can imagine a user having a lot of different types of those screens connected to the same machine, the case still seem to be very unlikely, on what we've seen so far, people will just use 1 screen! So the abstract class will largely do the job including on code reusability across projects. |
Most user need only 1 screen -- that's true. But here, in final product line, manager want to support many different kinds of chip to make the supply chain more flexible. That's mean the same app need to support multiple kinds of device, through each product will only connect to one, one of those supported. But I understand your concern, maybe a good document with lots of NotSupportedException is a possible solution. For example, the 8-segment device will throw NotSupportedException on all 16-segment writing methods. Although the current two devices only support 8-segment writing, it is also feasible to add methods with NotSupportedException to these two devices after a device supporting 16-segment is added later. Thanks for your idea. Actually, the NotSupportedException you mentioned inspired me. I put this work on the schedule for next week. :-) |
All interfaces are removed by moving methods into base class. |
Ok. Let me search how to do that and try to make it happen. |
It seems merged. Please proceed. |
Commenter does not have sufficient privileges for PR 1855 in repo dotnet/iot |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
I'll take a look at the pr again (don't recall what it was about) when I have time. I'm traveling right now |
Hey guys, any updates? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@scegg Sorry, this one got completely out of my view. I will try to check the remaining issues ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ellerbach Anything open from your end on this one?
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
src/devices/Device-Index.md
Outdated
@@ -119,7 +119,7 @@ | |||
* [TCA954X - TCA954X Low-Voltage Multi-Channel I2C Switch with Reset](Tca954x/README.md) | |||
* [TCS3472x Sensors](Tcs3472x/README.md) | |||
* [TLC1543 - 10-bit ADC with 11 input channels](Tlc1543/README.md) | |||
* [TM1637 - Segment Display](Tm1637/README.md) | |||
* [TM1637 - Segment Display](Tm16xx/README.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgrawehr please remove this file, the changes are done automatically. And it's the reason the build fails!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, "automatically" as in "when somebody manually runs the script", yes...
An update is required
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@krwq Anything open from your end? You have some very old change request in this PR. |
Review is outdated and more than a year old.
Add support to TM1650 chip.
Many thanks to the existed TM1637 code.
Microsoft Reviewers: Open in CodeFlow