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

Convert Protocol names to shared text #1078

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Convert Protocol names to shared text #1078

merged 4 commits into from
Apr 16, 2020

Conversation

crankyoldgit
Copy link
Owner

@crankyoldgit crankyoldgit commented Apr 10, 2020

  • Reduce string duplication by centralising protocol text.
  • Use a single array (double null terminated string) to store the text
    • Allows for much simpler, & more compact code.
    • Kudos to @s-hadinger for the idea.
  • Saves about ~3.8 kbytes.

FYI @s-hadinger

* Saves about 400 bytes.
@s-hadinger
Copy link
Contributor

Thanks David. I'm surprised that strcasecmp accepts PROGMEM parameter without crashing. Shouldn't you use strcasecmp_P instead?

Theo has a nice trick to save even more. In a nutshell, concatenate all strings of a specific type to a single string separate with |:

const PROGMEM char[] kDecodeTypeStr = "|"  kUnknownStr "|" kUnusedStr
             "|" kAirwellStr "|" kAiwaRcT501Str ; // and so on...

Then create an array of codes in parallel of the strings with the same indices:

const PROGMEM uint8_t[] kDecodeTypeCodes = { decode_type_t::UNKNOWN, decode_type_t::UNUSED,
             decode_type_t::AIRWELL, decode_type_t::AIWA_RC_T501,
             };  // and so on

Use the following for index to text (note the first string before the first | is a common prefix to all other strings, so leave it empty):

char* GetTextIndexed(char* destination, size_t destination_size, uint32_t index, const char* haystack)
{
  // Returns empty string if not found
  // Returns text of found
  char* write = destination;
  const char* read = haystack;

  index++;
  while (index--) {
    size_t size = destination_size -1;
    write = destination;
    char ch = '.';
    while ((ch != '\0') && (ch != '|')) {
      ch = pgm_read_byte(read++);
      if (size && (ch != '|'))  {
        *write++ = ch;
        size--;
      }
    }
    if (0 == ch) {
      if (index) {
        write = destination;
      }
      break;
    }
  }
  *write = '\0';
  return destination;
}

You can then have a strategy for matching similar to this one:

bool DecodeCommand(const char* haystack, void (* const MyCommand[])(void))
{
  GetTextIndexed(XdrvMailbox.command, CMDSZ, 0, haystack);  // Get prefix if available
  int prefix_length = strlen(XdrvMailbox.command);
  if (prefix_length) {
    char prefix[prefix_length +1];
    snprintf_P(prefix, sizeof(prefix), XdrvMailbox.topic);  // Copy prefix part only
    if (strcasecmp(prefix, XdrvMailbox.command)) {
      return false;                                         // Prefix not in command
    }
  }

   // retrieve the actual code from kDecodeTypeCodes
    return true;
  }
  return false;
}

The magic is that it works backwards, scan kDecodeTypeCodes for code to string, and use a first match to choose your preferred string in case a single code matches multiple strings.

This should save you a lot of code :)

@crankyoldgit
Copy link
Owner Author

@s-hadinger Thanks. I'll take a look at doing just that.

@crankyoldgit
Copy link
Owner Author

I'm surprised that strcasecmp accepts PROGMEM parameter without crashing. Shouldn't you use strcasecmp_P instead?

Interestingly, I tried using strcasecmp_P just now (I'm finally getting to this), and it added 500+ bytes to the binary size. So, if it ain't broke .. don't fix it?! /shrug.

* Simplify the conversion of str <-> decode_type_t
* Saves ~1.9k of flash
* Kudos to @s-hadinger for the idea!
@crankyoldgit
Copy link
Owner Author

This should save you a lot of code :)

@s-hadinger It did. I did something simpler as I don't have the same issues as Tasmota. I could do what you guys do, and shave a few more bytes, but this way makes the code more readable.
Anyway. Total saving is now approx 2.3k.

* Use a specially encoded string to store all the protocol names to save 
space.
* Saves an additional 750+ bytes.
* Saves another 700+ bytes.
@crankyoldgit
Copy link
Owner Author

Another revision, this time removing redundant strings, & using a single double "null terminated" string to store the text. All in all, an additional 1.5k more saving.

@s-hadinger
Copy link
Contributor

Wow. Great job. Let me try it in Tasmota

@s-hadinger
Copy link
Contributor

I confirm it's working on Tasmota, no crash due to PROGMEM.

It saves 3.4KB of flash. Do you plan to release a version v2.7.6 with the space saving feature?

@crankyoldgit
Copy link
Owner Author

Do you plan to release a version v2.7.6 with the space saving feature?

Yes, eventually.

@crankyoldgit
Copy link
Owner Author

7 days have passed. No un-addressed negative feedback. Merging.

@crankyoldgit crankyoldgit merged commit 636c3fe into master Apr 16, 2020
@crankyoldgit crankyoldgit deleted the save_space branch April 16, 2020 12:53
crankyoldgit added a commit that referenced this pull request Apr 25, 2020
_v2.7.6 (20200425)_

**[Features]**
- IRMQTTServer: Use more i18n text. (#1086)
- Convert Protocol names to shared text. Saves ~3k of flash. (#1078)
- Add Chinese translation (zh-CN) & add utf-8 support. (#1080, #1085)

**[Misc]**
- IRMQTTServer: Ensure MQTT_MAX_PACKET_SIZE is correctly set. (#1084)
- Add Italian locale to IRrecvDumpV2 platformio file.
@crankyoldgit crankyoldgit mentioned this pull request Apr 25, 2020
crankyoldgit added a commit that referenced this pull request Apr 25, 2020
_v2.7.6 (20200425)_

**[Features]**
- IRMQTTServer: Use more i18n text. (#1086)
- Convert Protocol names to shared text. Saves ~3k of flash. (#1078)
- Add Chinese translation (zh-CN) & add utf-8 support. (#1080, #1085)

**[Misc]**
- IRMQTTServer: Ensure MQTT_MAX_PACKET_SIZE is correctly set. (#1084)
- Add Italian locale to IRrecvDumpV2 platformio file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants