KurtE / ILI9341_t3n

Extended ILI9341_T3 library (Teensy) including all SPI buses, Frame buffer, plus
MIT License
50 stars 23 forks source link

fix DrawString 1 #34

Closed KurtE closed 3 years ago

KurtE commented 3 years ago

Before the only caller was drawString which padded the length passed in to add 2, so drawString1 reduced count by 2...

Instead pass in correct count from drawString...

I think this is the only thing needed to fix #33?

@mjs513 - does this look correct?

Could just remove it from PDF, but makes sense to have method where I pass in buffers as I always avoid using Strings...

Note: wonder why it was not also just an overload of the drawString method like:

int16_t ILI9341_t3n::drawString(const char string[], int16_t len, int poX, int poY) {

Also if this is correct, probably need to check/fix in ili9488_t3, HX....

mjs513 commented 3 years ago

@KurtE

Should work but drawString also gets called from drawNumber and drawFloat but if we overload it should be ok.

As to why it was not an overload function of the drawstring method - have no idea - could be at the time I was playing with it didn't register that I could just overload the function. I will give it a test later and see what happens.

If it works I will make the same change for ILI9488 and HX.. Can't remember if we put it the 7735 library - will have to double check.

KurtE commented 3 years ago

@mjs513 - for now no reason to overload the methods as if I did, would probably then leave in stub for those who may have called it.

Note: I am still thinking even with this "Fix" there could still be some issues, which I am not sure is worth fixing? Example: tft.drawString1("ABCDEFG", 4, 100, 100);

I tell it to output 4 characters, But the code starts off with:

int16_t ILI9488_t3::drawString1(char string[], int16_t len, int poX, int poY)
{
  int16_t sumX = 0;
  uint8_t padding = 1;

  uint16_t cwidth = strPixelLen(string); // Find the pixel width of the string in the font
  uint16_t cheight = textsize*8;

So it calls strPixelLen with the whole string expecting to find null character ending...

mjs513 commented 3 years ago

Going to give it a test now = was just looking at changes when you made the update.

mjs513 commented 3 years ago

`Note: I am still thinking even with this "Fix" there could still be some issues, which I am not sure is worth fixing? Example: tft.drawString1("ABCDEFG", 4, 100, 100);

I tell it to output 4 characters,`

Just used the "TFT_String_Align.ino" sketch to give it a test. The one issue I see is what you already noticed: uint16_t cwidth = strPixelLen(string); // Find the pixel width of the string in the font It leaves enough room for the whole length of the string. To fix you would have to create a new string with the length of what you are passing the function. To honest was never really designed to be set up that way, as a stand alone function. But then again might be interesting to do.

KurtE commented 3 years ago

@mjs513 again not sure worth extra work here, but:

Another option is to remove the len field from the drawString1,

It is used about 3 times in the function. Twice for loops: which could be changed from:

    for (uint8_t i = 0; i < len; i++) {
      drawChar((int16_t)(poX + sumX), (int16_t)poY, string[i], textcolor,

to something like:

    for (uint8_t i = 0;  string[i] != 0; i++) {
      drawChar((int16_t)(poX + sumX), (int16_t)poY, string[i], textcolor,

And a third place where the font is null:

  if (font == NULL) {
    for (uint8_t i = 0; i < len; i++) {
      drawChar((int16_t)(poX + sumX), (int16_t)poY, string[i], textcolor,
               textbgcolor, textsize_x, textsize_y);
      sumX += cwidth / len + padding;
    }

But if look at the helper function you have:

int16_t ILI9341_t3n::strPixelLen(const char *str) {
  //    //Serial.printf("strPixelLen %s\n", str);
  if (!str)
    return (0);
  if (gfxFont) {
    // BUGBUG:: just use the other function for now... May do this for all of
    // them...
    int16_t x, y;
    uint16_t w, h;
    getTextBounds(str, cursor_x, cursor_y, &x, &y, &w, &h);
    return w;
  }

  uint16_t len = 0, maxlen = 0;
  while (*str) {
    if (*str == '\n') {
      if (len > maxlen) {
        maxlen = len;
        len = 0;
      }
    } else {
      if (!font) {
        len += textsize_x * 6;
      } else {

So could we not just change the drawText1 code to use the textsize_x*6?

Of course there is yet another option, which is to have strPixelLen have a len parameter... but...

mjs513 commented 3 years ago

@KurtE As a kludge I added this function:

int16_t ILI9341_t3n::drawString(const char string[], int16_t len, int poX, int poY) {
  int16_t sumX = 0;
  uint8_t padding = 1 /*, baseline = 0*/;

char str_buffer[len];

size_t destination_size = sizeof (str_buffer);

strncpy(str_buffer, string, destination_size);
str_buffer[destination_size - 1] = '\0';

  uint16_t cwidth = strPixelLen(str_buffer); // Find the pixel width of the string in the font
  uint16_t cheight = textsize_y * 6;

  if (textdatum || padX) {
    switch (textdatum) {
    case TC_DATUM:
      poX -= cwidth / 2;
      padding += 1;
      break;
    case TR_DATUM:
      poX -= cwidth;
      padding += 2;
      break;
    case ML_DATUM:
      poY -= cheight / 2;
      // padding += 0;
      break;
    case MC_DATUM:
      poX -= cwidth / 2;
      poY -= cheight / 2;
      padding += 1;
      break;
    case MR_DATUM:
      poX -= cwidth;
      poY -= cheight / 2;
      padding += 2;
      break;
    case BL_DATUM:
      poY -= cheight;
      // padding += 0;
      break;
    case BC_DATUM:
      poX -= cwidth / 2;
      poY -= cheight;
      padding += 1;
      break;
    case BR_DATUM:
      poX -= cwidth;
      poY -= cheight;
      padding += 2;
      break;
    }
    // Check coordinates are OK, adjust if not
    if (poX < 0)
      poX = 0;
    if (poX + cwidth > width())
      poX = width() - cwidth;
    if (poY < 0)
      poY = 0;
    // if (poY+cheight-baseline >_height) poY = _height - cheight;
  }
  if (font == NULL) {
    for (uint8_t i = 0; i < len; i++) {
      drawChar((int16_t)(poX + sumX), (int16_t)poY, string[i], textcolor,
               textbgcolor, textsize_x, textsize_y);
      sumX += cwidth / len + padding;
    }
  } else {
    setCursor(poX, poY);
    for (uint8_t i = 0; i < len; i++) {
      drawFontChar(string[i]);
      setCursor(cursor_x, cursor_y);
    }
  }
  return sumX;
}

Of course I can simplify a bunch but again just a kludge to see if it would work and yes it does but not sure the utility.

Do you think its worth it?

mjs513 commented 3 years ago

Oops = looks like we just cross posted

KurtE commented 3 years ago

@mjs513 Yep we cross posted...

I posted yet another solution up on the branch: https://github.com/KurtE/ILI9341_t3n/tree/draw_string2 This one, I updated the function you call to strPixelLen to have an optional cb parameter which defaults to 0xffff Which should handle any number of characters.

The code simply then tests still for null character or cb==0 if else decrements the count.

mjs513 commented 3 years ago

@KurtE Just downloaded and tested and works fine with my test sketch. But don't really like calling it drawString1.

How about just overloading it to: int16_t ILI9341_t3n::drawString(const char string[], int16_t len, int poX, int poY) {

Only have to change drawString1 to drawString in drawString: From return drawString1(buffer, len-2, poX, poY); to return drawString(buffer, len-2, poX, poY);

KurtE commented 3 years ago

@mjs513 – I pushed that up… Look OK?

From: Mike S notifications@github.com Sent: Thursday, March 04, 2021 3:21 PM To: KurtE/ILI9341_t3n ILI9341_t3n@noreply.github.com Cc: KurtE kurte@rockisland.com; Mention mention@noreply.github.com Subject: Re: [KurtE/ILI9341_t3n] fix DrawString 1 (#34)

@KurtE https://github.com/KurtE Just downloaded and tested and works fine with my test sketch. But don't really like calling it drawString1.

How about just overloading it to: int16_t ILI9341_t3n::drawString(const char string[], int16_t len, int poX, int poY) {

Only have to change drawString1 to drawString in drawString: From return drawString1(buffer, len-2, poX, poY); to return drawString(buffer, len-2, poX, poY);

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/KurtE/ILI9341_t3n/pull/34#issuecomment-791023947 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AAL4MQBBLYBFVCGF5JCQ5LLTCAIVXANCNFSM4YTSYGMQ . https://github.com/notifications/beacon/AAL4MQF4BS4VXQL36G2AMG3TCAIVXA5CNFSM4YTSYGM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOF4TBCSY.gif

mjs513 commented 3 years ago

Yep - looks like what I just tested. Now guess to make the changes in the other libraries.