logo elektroda
logo elektroda
X
logo elektroda
Advanced Search
logo elektroda

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

max4elektroda 333 18
ADVERTISEMENT
Report a violation of the law
Reply | New topic
  • #1 21809489
    max4elektroda
    Level 23  
    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.
  • #3 21809523
    max4elektroda
    Level 23  
    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.
  • #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.
  • ADVERTISEMENT
  • #5 21811541
    max4elektroda
    Level 23  
    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
  • #6 21812520
    max4elektroda
    Level 23  
    Modified step 1 - moved code away form HAL code.

    I think it's o.k. to decide to find the pin index of an alias is not necessarily a HAL, but a pin related function.
    So it's moved to new_pins.c - making all the changes to include "hal_pins_generic.c" in Windows build - "new_pins.c" is always present.

    Tried step 2 - changing drivers which get their pins from CMD and not from an IORole.

    Using "Tokenizer_GetPin()" was already present in some drivers (drv_aht2x.c, drv_ds3231.c, drv_bmp280.c), I changed all other drivers I found using Tokenizer_GetArgInteger... functions to get the pins:

    drv_bmpi2c.c, drv_dmx512.c, drv_ds1820_common.c (only for testing timing), drv_ir2.c, drv_mcp9808.c, drv_pinMutex.c, drv_simpleEEPROM.c, drv_tca9554.c and drv_tm_gn_display_shared.c

    So now I think it's "testing time" ;-)
    Sadly I only own DS3231 and AHT2X from the list of devices using pin definition from "startdriver", so I can only confirm some few configuration.

    As an example BK7238 with AHT2X:
    Code: text Expand Select all Copy to clipboard
    
Debug:CMD:cmd [startdriver aht2x PWM1 pwm2]
Debug:CMD:Tokenizer_GetPin: Argument 1 (PWM1) - Returning index 7
Debug:CMD:Tokenizer_GetPin: Argument 2 (pwm2) - Returning index 8
Debug:CMD:Adding command SoftI2C_SetClkPeriod
Debug:CMD:Adding command AHT2X_Calibrate
Debug:CMD:Adding command AHT2X_Cycle
Debug:CMD:Adding command AHT2X_Measure
Debug:CMD:Adding command AHT2X_Reinit
Info:CMD:[WebApp Cmd 'startdriver aht2x PWM2 pwm1' Result] OK



    Screenshot of OpenBK7238 web interface showing AHT2X temperature and humidity data
  • ADVERTISEMENT
  • #7 21813800
    p.kaczmarek2
    Moderator Smart Home  
    Seems good and acceptable, just please remove testing remnant Win_DoSingleTest. Thank you!
    Helpful post? Buy me a coffee.
  • Helpful post
    #8 21814176
    max4elektroda
    Level 23  
    Removed all testing/debug code.
    Added some code to use SetPinRole with pin alias, too:



  • #9 21814300
    p.kaczmarek2
    Moderator Smart Home  
    Excellent! Merged.

    Thank you for your repeated helpful and useful contributions!

    Now, what can we do for OBK next?
    Helpful post? Buy me a coffee.
  • #10 21814308
    max4elektroda
    Level 23  
    Maybe you could take a look at PR#1936?
    For some time now we have restrictions on W600/W800 because newer (bigger) configs didn't work.
    If I'm not mistaken this was a quite "simple" issue (we overwrote config with flashvars when expanding config size).
    W800 and W600 tested o.k. by @divadiow and me.
  • #11 21815114
    p.kaczmarek2
    Moderator Smart Home  
    So the only drawback is that once after OTA flashvars will be reset, so device won't remember last LED state, etc?

    It's acceptable, if that's the case.

    Please confirm and I will merge it.
    Helpful post? Buy me a coffee.
  • ADVERTISEMENT
  • #12 21815316
    max4elektroda
    Level 23  
    Yes, that is the drawback, since flashvars are "moved".
    Maybe @divadiow can do a last test for this case?

    Flash release and set some vars.

    Flash PR version (vars should be "gone") - at least on the first time.
    If he did change the vars on the PR version before, they should be present again as on the last save with the PR version.

    Added after 21 [minutes]:

    Ah, and command line will be boosted over 1k due to larger size in actual config version.

    C source code snippet with macro definitions and character array declarations

    I didn't check if it will "survive", but I expect it
  • #13 21815371
    divadiow
    Level 38  
    ok.

    fresh 1.18.247, join W600 to AP as a client. set flags 12, 17, reboot - flags present
    OTA to 1936_merge_e347b264aba8

    on reboot:
    Code: Text
    Log in, to see the code


    new-device W600 AP is broadcasting. join wifi again so W600 is client. set flags 12, 17 again, reboot - flags still present
    OTA 1936_merge_e347b264aba8->1936_merge_e347b264aba8 - flags still present on reboot.

    Here is log from end of OTA to next boot

    Code: Text
    Log in, to see the code


    Added after 1 [minutes]:

    this never clears from 1
    Code: Text
    Log in, to see the code

    but it does count up on incomplete boot and go into safe mode as expected
  • #14 21815460
    max4elektroda
    Level 23  
    If it's not too much:
    Will startup command line from release be present after changing to new version?
    All other values of config are untouched, but this will start at the same address but be larger with new version...
    Thank you for your help!
  • #15 21815475
    divadiow
    Level 38  
    release-> PR OTA does wipe startup command
  • #16 21816080
    max4elektroda
    Level 23  
    Sorry, for the extra work: I didn't take into account that a bad crc / config will always set it to default, so all will be erased!
    So all data will be lost with this PR
  • #17 21816793
    divadiow
    Level 38  
    ok. can do again if there's any uncertainty
  • #18 21817012
    p.kaczmarek2
    Moderator Smart Home  
    Futhermore, this means we'll use ability to downgrade software version without config loss.
    Helpful post? Buy me a coffee.
  • #19 21817023
    max4elektroda
    Level 23  
    I'm thinking about an special upgrade handling to keep settings.
    But it might take some time, because I'm on a holiday now.
    Downgrade will be impossible (without losing config) since the old version doesn't know about the change. Maybe a special downgrade firmware, making sure you only loose the config in the addition range...
Reply | New topic
Report a violation of the law

Topic summary

The discussion centers on improving the pin naming and indexing system in OpenBeken firmware, specifically proposing a function like "PIN_FindPinIndexForRole_new(int role, int actual_index)" to streamline pin role verification and reduce complexity across drivers. The suggested approach involves first checking if the pin at a given index matches the desired role before searching, returning -1 if no valid pin is found, potentially allowing widespread driver updates with minimal code changes. However, concerns were raised about the minimal CPU cost of current pin retrieval methods, advocating to maintain the existing system to prioritize more impactful firmware improvements such as supporting pin string names in command text, enhancing stability, merging pull requests, and updating documentation. A related idea includes generalizing the DS1820_full driver’s pin handling and leveraging the existing "Tokenizer_GetPin()" function to accept both integer pin indexes and aliases, improving argument parsing flexibility. Caution is advised against enabling risky changes in main releases to avoid device bricking, especially in battery-powered variants with short startup times.
Summary generated by the language model.
Today's popular
ADVERTISEMENT