Create library
nseidle opened this issue · 24 comments
We should really write a library for this device. It would be much easier if a .begin() function took a TwoWire, Serial, or SPI stream and then over-wrote the .print function. We did this on the Qwiic OpenLog product.
I already started down that path a bit. I ported the LiquidCrystal library for Qwiic and the OpenLCD, which could be the I2C /TwoWire part. It has Print class implemented and all the LiquidCrystal examples ported. It's publicly available here on github: https://github.com/fourstix/QwiicSerLCD
I thought about expanding it to cover the other communication modes, but I was sure exactly how to best implement the selection of the communication interface type. Is it best done in the begin() statement or in a constructor? If there is a Sparkfun preferred way of doing it, I be glad to take a stab at it. I just need some direction on how Sparkfun would prefer things to be done.
Nice!
This is how we pass in Wire streams to libs so users can use Wire or WireX if they need it. You should be able to overload .begin with functions that have TwoWire, SPIClass, and Stream depending on what the user passes in (Wire, SPI, Serial).
The SCD30 library is an ok example of TwoWire class passing.
That's Great! I'll take a look at those examples and run with them. It looks like I should factor out the initialization of Wire and make that external to the class.
What would like the library class to be called? SerLiquidCrystal or something else? Also, any particular example or conventions I should follow? I can create a repository on Github, and then you could just fork it into Sparkfun once your happy with it. (I'm a bit new to Git and Github, so please let me know the preferred way to do things.)
Hmmm. LiquidCrystal has a begin function that takes two parameters, col and row, eg. begin(16, 2); I'm wondering if it would be better to put the stream logic in the form of a constructor, maybe delegating to an init method if needed. Something like SerLiquidCrystal(TwoWire& wireport).
SerLiquidCrystal(Wire) lcd;
lcd.begin(16, 2);
lcd.print("Hello, World!);
Library class: OpenLCD? I don't have anything better. Liquid Crystal is pretty well known and I don't want to confuse. Also, SerLCD reads as 'serial' LCD where it looks like you and I are both using Qwiic/I2C.
Sure, in the constructor works. I kind of prefer it in the .begin so that it's clear what options go with what stream. For example (and I haven't thought this through)
.begin(Wire, 0x35) - where 0x35 is the I2C address of the display.
.begin(SPI, 1000000) - where 1,000,000 is the SPI freq
.begin(16,2) is not needed as the SerLCD firmware auto-identifies the LCD size (via resistor).
Yep, eventual goal is lcd.print("Hello");
Constructor vs begin()
OK, that's a good point. Let's go with overloading begin() instead of overloading constructors. My main objection was the number of arguments in begin() if one includes the row and column as well as the arguments for the communication type. Since it's not necessary to specify row and column, it makes sense to put it there. (If one has more than 5 arguments, other programmers will point and laugh at you.)
Class name
I'll start making the changes in the QwiicLiquidCrystal class, and we can change the name later.
Unfortunately the name OpenLCD is already used for the firmware class name. It's generally a bad practice to name two classes the same thing, especially when they are somewhat related, but have different functions.
Personally, I like having "LiquidCrystal" in the class name, as this would be an implementation of that class. I'm leaning toward something like LiquidCrystal_OpenLCD . As a newbie, that would tell me this is the class to use my OpenLCD display with the LiquidCrystal functions.
I should have said I'll make the changes in the QwiicSerLCD repository for now. We can move them to a new repository with the right class name, later.
Come to think of it. Sparkfun_OpenLCD has a nice ring to it. That might make a good class name for the Library.
I've got the TwoWire and Serial passing done. Tested I2c via Qwiic and currently testing Serial using Software Serial on an Arduino Pro-Mini 3.3v / 8 MHz. Working to implement SPI next.
@nseidle I have SPI implemented and have tested Serial and SPI. It seems to be working. I2C still works fine as well. It's available for review at https://github.com/fourstix/QwiicSerLCD
I can re-name it Sparkfun_OpenLCD and create a library directory in OpenLCD and submit a pull request, if you think it's ready.
The complete first implementation of the library is ready. I'll create a pull request.
This is really good. I'm going through converting it to fit our lib format. Couple things:
- You shouldn't do a Wire.setClock() before lcd.begin in your examples because the .begin() does a Wire.begin which reset the clock back to 100kHz. The fix is to move Wire.setClock after lcd.begin.
- Lots of examples is really good. But it's hard to tell where to begin. We put 'ExampleX-' before each example name so it's clear how the examples escalate.
- I changed name to "SerLCD" (dropped Qwiic) since your lib looks to work (nice job!) with SPI. We'll make it work with serial as well. We'll have to tailor a few examples (sub directories) so user knows which examples go with which communication protocol.
Another trick:
#include <SerLCD.h> //Click here to get the library: http://librarymanager/All#SparkFun_SerLCD
Great points. I didn't realize Wire.begin() reset the clock. There probably are too many examples, since I ported every example from the LiquidCrystal library. Feel free to omit or add anything you want.
FWIW, I tested the library using SoftwareSerial and it seemed to work, so hopefully I implemented the Serial part correctly.
Why is there a delay inside the clear function? This causes your backlight example to run slowly/oddly.
https://github.com/sparkfun/OpenLCD/pull/13/files#diff-7c78931ad272942267b7f87c7b351b24R261
Sorry for all the discussion, I'm understanding more of your lib. There is another 50ms delay in the setBacklight function. This is ok, but not ideal. I'm starting to see that the SerLCD firmware (not your lib) is somewhat slow. I think it's related to the amount of time it takes to write to EEPROM the backlight values...but it shouldn't be this slow. I'm still digging. But can you explain why you've got delays in clear() and setBacklight()? My code doesn't work when they're removed... so again I think it's LCD firmware related.
Yes, I just added delays empirically to try and reduce flicker or to avoid over-running data sent to the display. For the backlight changes, it was done to avoid seeing the display messages. I believe the new set RGB backlight function would avoid them and the delay would probably be unnecessary. I ran a bunch of tests in a loop, and I think I had the display commands getting missed a few times. Sometimes the data bytes would appear as a Japanese character rather than be interpreted as a command escape sequence to set the cursor position. My guess was that I was just sending data too fast, and so I introduced a delay into the code.
I should have mentioned I was using the LiquidCrystal HelloWorld sketch to send commands over and over as a test. It doesn't clear the display, but moves the cursor back to the second line, first column and prints over the previous value. It prints the same number repeatedly until a second goes by.
Occasionally, say once every 20 minutes or so, I'd see a glitch, A Japanese character followed by the seconds value sticking out past the normal seconds count. It seemed to be the worst with SPI, better with SoftwareSerial and best with I2C. I thought it was random noise from my jumper wires.
Excellent. Yep, I agree. There's a
petSafeDelay(SYSTEM_MESSAGE_DELAY);
inside the changeBLBrightness() function in the OpenLCD firmware that's taking 500ms. That's the delay given me the headache.
There should be a faster way to change the backlight and your changeBacklightRGB() function is perfect. Do you foresee any issues removing this delay (and displayFrameBuffer()) from changeBackLightRGB()?
No, not at all. I literally copied them from the previous function (cargo cult programming) thinking that they had to be there for a reason.
That is the best use of cargo cult I think I've heard.
Ok, I think I finished re-arranging your library. It's over on this repo. Request into Arduino for lib manager inclusion is in the works.
I'm going to close these issues out. Let's open new ones on the library repo as necessary.