Closed hasenradball closed 4 years ago
When using the method setCursor()
with int
instead of byte
the error don't occur and compilation works with the ESP8266, ESP32 and Arduino's.
void SerLCD::setCursor(int col, int row)
Its there an need to set the types to int??
Here you see the Code which compiles fine with the in types in setCuror()
#include <Arduino.h>
#include <ESP8266WiFi.h>
#include <SerLCD.h>
#include <Wire.h>
// Klassen Instanzen
SerLCD lcd;
void update_display(void);
void setup() {
WiFi.mode(WIFI_OFF);
Serial.begin(115200);
while(!Serial);
Wire.begin(0, 2);
lcd.begin(Wire, 0x72);
lcd.clear();
lcd.print("Hello");
update_display();
}
void loop() {
// update_display();
// delay(50);
}
void update_display() {
lcd.clear();
lcd.print("Hello");
}
I actually don't know why we need an int type for setCursor()
to compile correctly.
Other libs like LyquidCrystal_I2C
also uses uint8_t type and compile for ESP8266, ESP32 and Arduino....
Hi, I wrote the original code which was harvested for this library. I did some research and the ESP libraries implement the min/max facility stricter than the older C type definitions in the Arduino code. The older definitions allow for arguments of different types to be compared,
The newer way uses templates and requires the arguments to be of matching types. The code uses numerals, which are are of implicit type int. So if the variable is a byte, ESP code won't compile even though the byte promotion is compatible. The fix is to explicitly cast the parameters to match in every min/max function call. In this case I'll cast the numerals to byte, ie replace 0 with (byte) 0.
Can you do me a favor? I'll fix the original code soon in my upstream repository in https://github.com/fourstix/QwiicSerLCD Can you pull it and test? I'm going to open an issue there to fix this under. After testing I'll create a pull request for Sparkfun.
It's a bit easier for Sparkfun if we do this kind of dance, rather than pushing code changes directly here.
Hi thanks for the fast reply. Yes I can do the favor for you. Give me a info when the code ist availabe. I will be able to test on the esp8266.
Where does the impicit type come from? From the #define?
Hi, The fix is now available. I tested it by compiling with board set to Sparkfrun 8266 Thing. The problem comes from a statement such as min(variable, 0).
The literal string "0" in C++ has an implicit type of int. Normally that's not a big deal in old C compiled code where min/max were implemented by a #define statement. Arguments were simply promoted and it was the programmers responsibility to use constants within the type. But when using templates to define these functions, then a type check is done at compile time, and if the types do not match, an error is thrown during compile time.
(Kids today with their fast cars and fancy type checking......) :-)
I just re-based my repository with some earlier updates from Sparkfun and created pull request #7 with the changes to fix this issue.
What do you think to use an int type instead of byte in the method. Many micros today are faster when dealing with int instead of bytes
I think it's a bad idea. Here's why: 1) The type int is frankly the wrong type, because the only allowable values for the parameters are small values, well below 255. Type int implies parameter values that can be higher than that, Those higher values are illegal values, so type int is misleading to the user. Type byte signals to the user that only small values are allowed.
2) The functional contract has already been published with byte for the parameter types and changing it to int could disrupt other code. Its generally bad practice to change a header definition after it's public, so I avoid it whenever I can.
3) Type int is the type that can potentially cause trouble and speed loss. It's meaning can vary slightly, platform to platform. The type byte is precise, and according to the Arduino forums, byte is just as fast, even faster in some cases. Plus type byte is smaller and saves precious space. Someone using this library might need every precious byte they can get. Type byte saves space.
One could probably argue type uint8_t vs byte, but that's another debate. :-) Current fashion seems to prefer type byte.
Hi, the change:
/*
* Set the cursor position to a particular column and row.
*
* column - byte 0 to 19
* row - byte 0 to 3
*
* returns: boolean true if cursor set.
*/
void SerLCD::setCursor(byte col, byte row)
{
int row_offsets[] = {0x00, 0x40, 0x14, 0x54};
//kepp variables in bounds
row = max((byte) 0, row); //row cannot be less than 0
row = min(row, (byte) (MAX_ROWS - 1)); //row cannot be greater than max rows
//send the command
specialCommand(LCD_SETDDRAMADDR | (col + row_offsets[row]));
} // setCursor
was tested with Arduino 1.8.12 and compile works for:
BUT!!!
have a look to the description:
returns: boolean true if cursor set.
The code will never return any value!
Good catch. The comment is wrong. The function type is void so it returns nothing. I'll fix the comment and do a new pull request.
Thank you very much for testing that and taking the time to look at the code.
@hasenradball By the way, one thing I should have mentioned earlier, you are entirely correct on the point that 16 bit operations are usually faster than 8 bit.
But the cool thing is, that the compiler will optimize the code and can internally change a narrow type into a wider type in the output object code when that would be more efficient. That's why one can use byte as a type without too much worry over efficiency of 8 bit operations vs 16 bit.
Awesome. Thanks for reporting and thanks for the PR fourstix! Should be fixed in v1.0.8.
With a simple Arduino sketch I get an Error during compile.
I don't not know what I make wrong?
I get the following Error with the ESP8266: