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

Use int16_t for pins instead of int8_t #399

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asukiaaa
Copy link

@asukiaaa asukiaaa commented Sep 6, 2022

I wanted use LCD and Adafruit_ILI9341 with using PA4 of STM32F7 as CS pin.
However PA4 is 205 so it cannot be handled with int8_t type so I change type for pins to int16_t.
I could use PA4 as CS pin with this code.

Thank you for sharing useful library.

@BillyDonahue
Copy link
Contributor

(lurker thought)
Would it be possible to change instead to uint8_t? Then the expansion into bit-8 wouldn't change the memory footprint.

@dhalbert
Copy link

dhalbert commented Sep 6, 2022

Duplicating this comment: adafruit/Adafruit_ILI9341#82 (comment)

Pin numbers in Arduino are generally uint8_t, and the lower-level code generally assumes that. In this library and the other one you PR'd, int8_t is used to allow specifying -1 to indicate "no pin".

It might have been better for this and the other library to use uint8_t and to use 0xff to indicate "no pin". But that did not happen in the past. It would be an incompatible change to fix that.

In your particular case, have you tried just casting: (int8_t) 204 ? That may allow you to use the pin without changing the signatures of these routines.

@asukiaaa
Copy link
Author

asukiaaa commented Sep 8, 2022

Would it be possible to change instead to uint8_t? Then the expansion into bit-8 wouldn't change the memory footprint.

It's one approach but this library detect pin is available if the number is 0 or more so we need to add some flags to detect no pins.
https://github.com/adafruit/Adafruit-GFX-Library/blob/master/Adafruit_SPITFT.cpp#L139

It might have been better for this and the other library to use uint8_t and to use 0xff to indicate "no pin". But that did not happen in the past. It would be an incompatible change to fix that.

So?
These libraries use -1 to detect no pin.
https://github.com/adafruit/Adafruit_ILI9341/blob/master/Adafruit_ILI9341.h#L137
https://github.com/adafruit/Adafruit-GFX-Library/blob/master/Adafruit_SPITFT.h#L131

It will become breaking change but I think that it's better to support more than 127 pins.

In your particular case, have you tried just casting: (int8_t) 204 ?

Range of int8_t is between -128 and 127 so (int8_t) 204 become -54.

@dhalbert
Copy link

dhalbert commented Sep 8, 2022

Range of int8_t is between -128 and 127 so (int8_t) 204 become -54.

Right, the bit representation is the same. It will be converted to a uint_8t at some point in the SPI library, etc. in STMduino. Could you try that and see if it works?

@asukiaaa
Copy link
Author

asukiaaa commented Sep 8, 2022

I tried but does not work with using released version.

#define PIN_LCD_DC PD15
#define PIN_LCD_A_CS PD14
#define PIN_LCD_A_RST PF12
#define PIN_LCD_B_CS PA4
#define PIN_LCD_B_RST PA3

Adafruit_ILI9341 lcdA(PIN_LCD_A_CS, PIN_LCD_DC, PIN_LCD_A_RST);
Adafruit_ILI9341 lcdB((uint8_t)PIN_LCD_B_CS, PIN_LCD_DC, (uint8_t)PIN_LCD_B_RST);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants