-
Notifications
You must be signed in to change notification settings - Fork 7
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
Allow to couple/uncouple CE pin #23
Conversation
Part I/II
Part II/II To use couple function, just use "couple()" in the lcd initilization code. Then clock is always enabled which makes drawing a bit faster on regular pins and helps reducing interference when using two or more displays on the same data pins.
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.
Hi Jonas, thanks for your contribution.
Can you explain how would the new functions would be used in practice with a bare minimum example?
src/Nokia_LCD.h
Outdated
void couple(bool yes = true); | ||
void uncouple(); |
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.
Is couple(false)
the same as uncouple
?
If so, then let's keep only one.
src/Nokia_LCD.cpp
Outdated
if (!coupled) | ||
digitalWrite(kCe_pin, LOW); |
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.
Consistent styling:
if (!coupled) | |
digitalWrite(kCe_pin, LOW); | |
if (!coupled) { | |
digitalWrite(kCe_pin, LOW); | |
} |
src/Nokia_LCD.cpp
Outdated
if (yes && !coupled) { | ||
digitalWrite(kCe_pin, LOW); | ||
} | ||
else if (!yes && coupled) { | ||
digitalWrite(kCe_pin, HIGH); | ||
} | ||
coupled = yes; |
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.
Hmm, couldn't this be changed to something simpler, i.e.:
if (yes && !coupled) { | |
digitalWrite(kCe_pin, LOW); | |
} | |
else if (!yes && coupled) { | |
digitalWrite(kCe_pin, HIGH); | |
} | |
coupled = yes; | |
digitalWrite(kCe_pin, yes ? LOW : HIGH); | |
coupled = yes; |
src/Nokia_LCD.cpp
Outdated
if (!coupled) | ||
digitalWrite(kCe_pin, HIGH); |
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.
Consistent styling:
if (!coupled) | |
digitalWrite(kCe_pin, HIGH); | |
if (!coupled) { | |
digitalWrite(kCe_pin, HIGH); | |
} |
src/Nokia_LCD.h
Outdated
/** | ||
* couple()/uncouple() is useful for improving speed and | ||
* for accessing two or more displays on the same pins | ||
* (but different CE pins) | ||
*/ |
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.
The claims are that using these two new functions:
- Improve speed
- Allow accessing more than 1 display on the same pins
I can understand the 2nd point, but can you elaborate more on the 1st?
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.
Two answer all your remarks and maybe more:
- Why this proposal
I was experimenting with two displays. But they constantly resisted to work as expected. I suspect electrical interference between the CE lines. Another advantage is that the contrast setting menu can be shown on all displays the same time so if your application misconfigured one LCD but shows the contrast setting menu on the other LCD too you still can read what's going on.
- How-to use
The key of couple() is to stop the send() method from modifying the CE pin. Obviously you have to manage it by yourself then.
The bonus of couple() is that you can switch from one display to another without changing all these lcd.print() invocations in main and subroutines. I use this - but to be frank it would be more clear to wrap a digitalWrite such as activate_lcd1() {digitalWrite(CEofLCD1, LOW);} - I probably will switch to this soon for sake of clarity.
- Examples
See
a) Full http://z-info.jbechtel.de/srv/23kopierer_savev179web.ino
b) Reduced http://z-info.jbechtel.de/srv/23kopierer_web_short.ino
c) Mini http://z-info.jbechtel.de/srv/23kopierer_web_veryshort.ino
The a) example is an actually working one. But it contains lots of code which you don't need for LCD (and also lots of dead code).
The b) example is untested. I just took the a) example and removed everything which is not contrast setup menu/counter printer.
The c) example is pretty minimal, just has the initialization phase of b) example. Also untested.
Please don't mind the "lcd.uncouple()" in all a)b)c) examples initialization. It is wrong and most probably should be just removed.
Also please don't mind that it is not neccessary to instantiate lcdL and lcdR objects. lcd object is just enough. All lcdL and lcdR tasks could be surrounded by activate_lcd1() like wrappers (see above 2.).
- Improve speed
It was just my feeling that on Arduino the PIN access is pretty slow, compared to directly coding for ATMega. Web search engine results support this. (digitalWrite() seems to switch off PWM first, etc.). So omitting digitalWrite(), even just the surrounding one for CE, helps a bit. Of course one could be pretty much faster when not-at-all using digitalWrite() but reducint calls is not so bad, either.
I have incorporated most of your suggestions and expanded the documentation comment in Nokia_LCD.h. Is it better than before?
As suggested by pull request review
As suggested by pull request review
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.
I think we are getting somewhere :)
src/Nokia_LCD.h
Outdated
* ac) reducing electrical interference in case it happens that one CE line | ||
* overspeaks to the other CE line |
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.
Sounds a bit farfetched but I haven't tried it so I guess you have a point.
However, it feels like not only the electrical engineering aspects are outside the scope of this library, but -more importantly- this isn't some information we should include in the API documentation.
src/Nokia_LCD.h
Outdated
* ad) manage CE pin by different code | ||
* lcd.couple(); // once and forever, now someone else has to manage | ||
* // CE pin |
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.
I think I understand this one, but does it need to be in the API documentation?
src/Nokia_LCD.h
Outdated
* ad) manage CE pin by different code | ||
* lcd.couple(); // once and forever, now someone else has to manage | ||
* // CE pin | ||
* b) improving speed by ~10% (16 instead of 18 digitalWrite() per data/command byte) |
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.
While there are 10% less instructions that doesn't necessarily mean it's 10% faster, especially since branches have been introduced to the code.
Regardless, any performance claim should be backed up by data and I'd argue that even if there is a 10% performance boost, it isn't worth mentioning in the API. :)
platisds' points are reasonable; you can manage to use couple()/uncouple() only when needed; the API description can be a straightforward functional description with little example. Co-authored-by: Dimitris Platis <platisd@users.noreply.github.com>
It is right that the performance claim isn't from measurements and that the interface description is better brief than possibly wrong. One could argue how much access to inner variables should be possible, but of course it's not unacceptable to make access private. |
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.
👍 🎉
👍 Thank you! Makes exchange of the .ino file easier between my build pipeline (Makefile) and the project initiators build pipeline (Arduino IDE) without any explanation. |
As signalized I have checked the "electrical interference" issue. Maybe it isn't related to electrical interference, at least I couldn't reproduce this. It's probably just a question of reset pins. The begin() method does set reset pin to low (and high) and then writes makes necessary initialization bytes. So there are following variants to share pins of two LCDs:
So now I'm done with that story. |
Thanks for the follow-up @jjtmp 💯 |
No description provided.