logo elektroda
logo elektroda
X
logo elektroda
Advanced Search
logo elektroda

How to Implement PIN_FindPinIndexForRole_new() Function in OpenBeken for Pin Management

max4elektroda 66 4
ADVERTISEMENT
Report a violation of the law
Reply | New topic
  • #1 21809489
    max4elektroda
    Level 22  
    In fact I was generally speaking, not taking into account the presence of the relay config where we would need the same for all three roles, which might make the approach "at least as complicated" as simply always checking the pins...

    Maybe we could use a new or changed function (like) "PIN_FindPinIndexForRole()":
    A "PIN_FindPinIndexForRole_new(int role, int actual_index)" could help doing the same for all drivers.
    First checking, if "g_cfg.pins.roles[actual_index] == role" an only if not searching.
    If we return a predefined "-1" (no valid pin) in case of an unsuccessful search, we might even change a lot of drivers to use this new function with only little changes.

    I didn't check if we might even replace the original function by changing the code...
    (in case the call uses "defaultIndexToReturnIfNotFound" to return a valid pin, this would need some special treatment in the calling code...)
    From Display to Complete System: How the Solution Team Creates Value at Unisystem [ad]
  • ADVERTISEMENT
  • #2 21809493
    p.kaczmarek2
    Moderator Smart Home  
    Getting pins costs so few CPU cycles that I would strongly suggest to leave it as is. There are more important quality of live fixes that we should do, including:
    - support pin string names for drivers that take pin indexes in the command text
    - general stability fixes
    - reviewing and testing which PRs can be merged
    - updating docs, etc, docs page for variants
    etc, etc.

    The rule of thumb should be making firmware more user friendly, so if we can allow users changing pins at runtime at the cost of few cpu cycles, then let it be.

    We also now have a build for battery powered devices (variant) with short startup time, maybe some of @protectivedad fixes can be enabled there? Just please, be careful, don't enable risky things in main release, we can't just go around bricking people's devices. I already saw how removing startup delay for Beken breaks SOME of user devices while other devices are working good.

    Added after 1 [minutes]:

    This is the battery variant I mention:
    How to Implement PIN_FindPinIndexForRole_new() Function in OpenBeken for Pin Management
    but we need docs for that... variants.md page?
    Helpful post? Buy me a coffee.
  • ADVERTISEMENT
  • #3 21809523
    max4elektroda
    Level 22  
    p.kaczmarek2 wrote:
    - support pin string names for drivers that take pin indexes in the command text

    You mean something like in DS1820_full driver made more general?

    How to Implement PIN_FindPinIndexForRole_new() Function in OpenBeken for Pin Management

    Added after 40 [minutes]:

    Will try a PR for that later

    Added after 10 [minutes]:

    Since there already is "Tokenizer_GetPin()" maybe we can just check for the argument being an int like

    Code: text Expand Select all Copy to clipboard
    int Tokenizer_GetPin(int i, int def) {
   int r;

   if (g_numArgs <= i) {
      return def;
   }
//   return HAL_PIN_Find(g_args[i]);
   return Tokenizer_IsArgInteger(g_args[i]) ? Tokenizer_GetArgInteger(g_args[g_args[i]]) : HAL_PIN_Find(Tokenizer_GetArg(g_args[i]));
}


    so it would work for an argument for pins as int or alias.


    Or did you mean "expand existing drivers" to use this?

    Added after 41 [minutes]:

    p.kaczmarek2 wrote:
    Getting pins costs so few CPU cycles that I would strongly suggest to leave it as is.

    I respect your decision, but let me make one "last" suggestion to slightly alter PIN_FindPinIndexForRole() without changing behavior.

    From
    Code: text Expand Select all Copy to clipboard
    int PIN_FindPinIndexForRole(int role, int defaultIndexToReturnIfNotFound) {
   int i;

   for (i = 0; i < PLATFORM_GPIO_MAX; i++) {
      if (g_cfg.pins.roles[i] == role)
         return i;
   }
   return defaultIndexToReturnIfNotFound;
}


    To
    Code: text Expand Select all Copy to clipboard
    
int PIN_FindPinIndexForRole(int role, int defaultIndexToReturnIfNotFound) {
   int i;

   if ( defaultIndexToReturnIfNotFound >=0 && defaultIndexToReturnIfNotFound<PLATFORM_GPIO_MAX && g_cfg.pins.roles[defaultIndexToReturnIfNotFound] == role) return  defaultIndexToReturnIfNotFound;
   for (i = 0; i < PLATFORM_GPIO_MAX; i++) {
      if (g_cfg.pins.roles[i] == role)
         return i;
   }
   return defaultIndexToReturnIfNotFound;
}


    This way, when giving actual pin as "default" (setting it to e.g. -1 on init) we can "speed up" (or prevent) search if there's no change.
    Using actual pin as "default" is actually quite often used, you just need to accept you won't recognize if a role is later deleted during operation.

    Added after 6 [hours] 49 [minutes]:

    I realized there are very few platforms with a realization of "HAL_PIN_Find()" which might was what you referred to?
    I made a PR PR#1948 to add it for all platforms it was missing (without BL602 which has no aliases at all).

    I also made some extensions to windows hal_pins.c so we can test here.
    The challenge is, that quite some platforms have not only "simple" alias like "A12" or "RX1" but more complex ones like "PB0 (L_TX)" or even "IO2 (B2/TX1)".
    So we need a bit more complex function to match "IO2" and "B2" and "TX1" (so search substrings) and make also sure, we won't match substrings not representing a string ("A1" is also in "A11").

    I hope I have a correct implementation now, it's also included in Windows simulator (including a simple selftest).

    I also added a test command to play/verify function:

    for windows I made some additional "challenging" alias definition for index 13-15

    Code: text Expand Select all Copy to clipboard
       const char *HAL_PIN_GetPinNameAlias(int index) {
   // some of pins have special roles
   if (index == 23)
      return "ADC3";
   if (index == 26)
      return "PWM5";
   if (index == 24)
      return "PWM4";
   if (index == 6)
      return "PWM0";
   if (index == 7)
      return "PWM1";
   if (index == 0)
      return "TXD2";
   if (index == 1)
      return "RXD2";
   if (index == 9)
      return "PWM3";
   if (index == 8)
      return "PWM2";
   if (index == 10)
      return "RXD1";
   if (index == 11)
      return "TXD1";
   if (index == 13)
      return "IO13/Test13/RRX13 XX13";
   if (index == 15)
      return "X13(RX13)";
   if (index == 14)
      return "IO14 Test14 (X14/RX14/XX14)";
   return "N/A";
}



    so there are some "difficult" cases to test:

    X13 shall be index 15, not 13 (XX13 or RX13 shall not match)
    RX13 shall be index 15, not 13 (RRX13 shall not match)

    Looks o.k for me:

    backlog getPin Test13 ; getPin X13; getPin RX13; getPin rx13, getPin XX14; getPin TXD2 ; getPin ADC3:
    Code: text Expand Select all Copy to clipboard
    
Info:CMD:[WebApp Cmd 'logfeature 1 0' Result] OK
Info:CMD:Pin index for Test13 is 13
Info:CMD:Pin index for X13 is 15
Info:CMD:Pin index for RX13 is 15
Info:CMD:Pin index for rx13, is -1
Info:CMD:Pin index for TXD2 is 0
Info:CMD:Pin index for ADC3 is 23
Info:CMD:[WebApp Cmd 'backlog getPin Test13 ; getPin X13; getPin RX13; getPin rx13, getPin XX14; getPin TXD2 ; getPin ADC3' Result] OK


    Added after 2 [minutes]:

    @p.kaczmarek2 : feel free to delete/move the last posts which are not really related to battery management - sorry for hijacking this thread.
  • ADVERTISEMENT
  • #4 21811509
    p.kaczmarek2
    Moderator Smart Home  
    I think the change is acceptable and I could merge it even today, however I noticed that helper method for finding pin name in alias seem to repeat in every HAL. Would you be able to move it to a common file, let's say, the one where other string utils are? new_common or something, how it was called?

    If it's not used in given platform, it will be simply stripped by the linker, so it won't hurt to have it.

    Also, are all drivers using Tokenizer_GetPin when required?
    Helpful post? Buy me a coffee.
  • #5 21811541
    max4elektroda
    Level 22  
    p.kaczmarek2 wrote:
    I noticed that helper method for finding pin name in alias seem to repeat in every HAL. Would you be able to move it to a common file, let's say, the one where other string utils are? new_common or something, how it was called?

    Yes, I saw that and already thought about, if we can't move the complete function to the hal_generic?
    We don't have access to the g_num_pins of most platforms, but if we would use "PLATFORM_GPIO_MAX" for all we should be fine, since "HAL_PIN_GetPinNameAlias(i)" is also available for all platforms.
    If we also modify the "simple" one (only one single alias string like e.g. in W800, LN882H ...) we might even put both there and with a #ifdef decide, which one to use for the actual platform.
    It wouldn't save memory, but would make it much easier to maintain e.g. if someone comes up with a smarter/better idea for this.

    Let me also check, if there maybe already is something similar in new_common or we can put it there for general use.

    I'll check and give feedback.


    p.kaczmarek2 wrote:
    Also, are all drivers using Tokenizer_GetPin when required?


    It's the same for the drivers, that's a complete todo now, not even started, I'll have to check how they get the argument during start.

    Added after 4 [hours] 53 [minutes]:

    For now I did step one:

    Moved the code to generic (hal_pins_generic.c) and removed it for the few platforms which already had it implemented.

    Made the call case insensitive (by introducing a new, case insensitive version of strstr() in new_common): "char *wal_stristr(const char *haystack, const char *needle);"

    I had to include the src\hal\generic\hal_pins_generic.c in Windows build, too, but to hide all the weak functions there for Windows build - compiler threw errors).

    I also enabled TEST for all platforms, which needed some changes for ESPDIF platforms (comment out recursive stack-overflow-test).

    I think it's now o.k. to test the platforms I don't own to do some tests with command "getPin <alias>" to find out if it's working as expected.
    Maybe @divadiow would be so kind?
    just try out if the command "getPin <ailas>" works (returning the correct pin index) on the platform.
    Maybe also try some "errors" (asking for non-existing pins or only parts (even if there is TX1, the command "getPin Tx" should return -1 because it's no full match.
    Especially the platforms with multiple aliases per pin should work with all names for a pin that's why for testing I made these funny pins for windows:


    Interface section with dropdowns and pin names P13–P15

    For me Windows seems good, it's working case insensitive:
    Code: text Expand Select all Copy to clipboard
    Info:CMD:Pin index for Test13 is 13
Info:CMD:Pin index for X13 is 15
Info:CMD:Pin index for RX13 is 15
Info:CMD:Pin index for RRX13 is 13
Info:CMD:Pin index for 15t is 13
Info:CMD:Pin index for rx13 is 15
Info:CMD:Pin index for XX14 is 14
Info:CMD:Pin index for TXD2 is 0
Info:CMD:Pin index for Adc3 is 23
Info:CMD:Pin index for ad is -1
Reply | New topic
Report a violation of the law
Today's popular
ADVERTISEMENT