Soft-off after 10 seconds not earlier

[amandel]

No description provided.

[amandel]

Arquivo: src/OpenBikeSensorFirmware.cpp

Resumo da Revisão:

A alteração aumenta o tempo necessário para desligar o OBSPro por software de 2 segundos para 10 segundos. A mudança em si parece correta, mas faltam informações cruciais para uma revisão completa.

Comentários:

• Justificativa: Falta uma justificativa clara para essa mudança. Por que 10 segundos é o tempo ideal? Qual o problema que essa alteração resolve? Adicionar essa informação no comentário e, idealmente, na descrição do Pull Request, facilita a revisão e o entendimento da alteração.
• Teste: Como essa alteração foi testada? É importante incluir informações sobre os testes realizados para garantir que a mudança funciona como esperado e não introduz novos problemas. Por exemplo, descreva o procedimento de teste para replicar o desligamento por software.
• Constante Mágica: Os valores 100 e 50 (anteriormente) são "números mágicos". Seria melhor substituí-los por constantes com nomes descritivos, como BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN e, talvez, manter o valor original em outra constante para eventualmente comparação/restauração. Isso melhora a legibilidade e facilita futuras modificações. O ideal seria calcular o threshold a partir de um SHUTDOWN_TIME_SECONDS para facilitar manutenção e clareza.
• Segurança (Potencial Problema - dependendo da aplicação): Aumentar o tempo para desligamento pode ter implicações de segurança, dependendo do contexto de uso do dispositivo. Se o dispositivo for facilmente acessível por terceiros, um tempo de desligamento longo pode permitir que alguém o desligue indevidamente. Considere os possíveis cenários de uso e se esse tempo de desligamento aumentado representa um risco. Se sim, documente o risco e justifique a mudança com mais detalhes.

Sugestão de Código Aprimorado:

// Definir constantes para melhorar a legibilidade e manutenção
const uint8_t SHUTDOWN_TIME_SECONDS = 10; // Tempo em segundos para desligamento
const uint8_t TIMER_INTERVAL_MS = 20;     // Intervalo do timer em milissegundos (assumindo que powerKeepAliveTimerISR é chamado a cada 20ms)
const uint16_t BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN = (SHUTDOWN_TIME_SECONDS * 1000) / TIMER_INTERVAL_MS;

// ... dentro de powerKeepAliveTimerISR() ...

// Soft power-off OBSPro when button is pressed for more than SHUTDOWN_TIME_SECONDS seconds
if (button.read()) {
  if (buttonPressedCounter < 255) {
    buttonPressedCounter++;
  }
} else {
  buttonPressedCounter = 0;
}

if (shutdownState == 0 && buttonPressedCounter >= BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN) {
  shutdownState = 1;
}

// ... restante do código ...

[amandel]

Review Summary:

This change improves the shutdown logic by using the JC_Button library's built-in functionality for detecting long presses, eliminating the need for manual button press counting. This makes the code cleaner, easier to understand, and less prone to errors. However, there's a potential issue with the logic and a minor improvement possible.

File: src/OpenBikeSensorFirmware.cpp

Issues:

1. Potential Race Condition (Major): In powerKeepAliveTimerISR(), the digitalRead(IP5306_BUTTON) is called multiple times. Since this is an interrupt service routine, the button state could theoretically change between these reads, leading to unexpected behavior. It's best to read the button state only once and store it in a local variable. More importantly, directly using digitalRead() within an ISR meant for power management is risky. You should be using the debounced button state provided by the JC_Button library.

2. Unclear Button Behavior (Minor): The code now shuts down when the button is pressed for an extended period. Typically, devices shut down when the button is released after a long press. This might be confusing for users.

Suggestions:

1. Fix Race Condition and Use Debounced State: Read the button state once at the beginning of the ISR using button.read() and store it in a variable. Use that variable for subsequent checks. Replace all instances of digitalRead(IP5306_BUTTON) inside powerKeepAliveTimerISR() with accesses to the JC_Button library's state. Example:

static void powerKeepAliveTimerISR()
{
  bool buttonState = button.read(); // Read the button state ONCE.
  unsigned long timeSinceLastPowerKeepAlive = millis() - timeOfLastPowerKeepAlive;

  if (shutdownState == 0) {
    if (!buttonState && timeSinceLastPowerKeepAlive > POWER_KEEP_ALIVE_INTERVAL_MS) {
      timeOfLastPowerKeepAlive = millis();
      digitalWrite(IP5306_BUTTON, HIGH);
    } else if (buttonState && timeSinceLastPowerKeepAlive > 300) {
      timeOfLastPowerKeepAlive = millis();
      digitalWrite(IP5306_BUTTON, LOW);
    }
  }

  if (shutdownState == 0 && button.wasReleased() && button.getPreviousStateMillis() >= BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS) {
    shutdownState = 1;
  }

  // ... rest of the ISR
}

2. Clarify Shutdown Behavior (Consider User Experience): Consider changing the shutdown trigger to button.wasReleased() and button.getPreviousStateMillis(). This ensures the device shuts down when the button is released after being held for a specified duration, which is more conventional and less likely to be triggered accidentally. This is reflected in the suggested code above.

3. (Optional) Symbolic Constant for 300ms: The magic number 300 should be replaced with a symbolic constant like POWER_TOGGLE_INTERVAL_MS for better readability and maintainability.

By addressing these issues, the code will become more robust, easier to understand, and provide a better user experience.

[amandel]

Review Summary:

This change improves the shutdown logic by using the button library's built-in functionality for tracking button press duration, simplifying the code and making it more reliable. It also clarifies the power keep-alive logic. However, there's a potential issue with the button type and debouncing that needs to be addressed.

File: src/OpenBikeSensorFirmware.cpp

Issues and Suggestions:

1. Button Debouncing: The code relies on digitalRead(IP5306_BUTTON) and button.read() for button state detection. This is susceptible to button bounce, which can lead to spurious shutdowns or incorrect power keep-alive behavior. Explicit debouncing should be implemented, either using hardware debouncing with a capacitor or software debouncing using a timer and state machine.

   • Suggestion: Consider using the Bounce2 library for software debouncing. This library handles the complexities of debouncing effectively. Example implementation assuming button is a Bounce object:

     #include <Bounce2.h>
     
     // ... (In setup)
     button.attach(IP5306_BUTTON, INPUT_PULLUP); // Assuming IP5306_BUTTON is configured with an internal pull-up resistor
     button.interval(5); // Debounce interval in milliseconds
     
     // ... (In powerKeepAliveTimerISR)
     button.update();
     bool ip5306ButtonState = !button.read(); // Invert if button pulls low when pressed

2. Button Type Assumption: The code uses button.getCurrentStateMillis() which suggests that the button object is an instance of a class like Bounce or EasyButton that tracks press duration. However, the previous code used digitalRead and a counter, implying a simple digital pin. There's a mismatch here. The type of button should be explicitly defined and initialized correctly.

   • Suggestion: Confirm the type of button and ensure it's appropriately initialized in the setup() function. If it's a debouncing library like Bounce or EasyButton, use its methods consistently for both shutdown and power keep-alive logic. If it's not such a library, update the code in the PR to avoid assumptions and explicitly handle time management in the ISR.

3. Clarity on IP5306_BUTTON Function: The code toggles IP5306_BUTTON while also using it to detect user input for shutdown. This dual-use of the pin is confusing. Clarify the role of IP5306_BUTTON. Is it solely for power management communication, or is it also a user input button? If it's both, consider separating these functionalities to different pins to avoid conflicts and improve readability.

4. Magic Number: The value 300 in the else if(ip5306ButtonState && timeSinceLastPowerKeepAlive > 300) appears to be a magic number. Define a constant for this value with a descriptive name to improve code readability and maintainability.

   • Suggestion:

     const uint16_t POWER_KEEP_ALIVE_TOGGLE_DELAY_MS = 300;
     // ...
     else if(ip5306ButtonState && timeSinceLastPowerKeepAlive > POWER_KEEP_ALIVE_TOGGLE_DELAY_MS)

By addressing these issues, the PR will be significantly improved in terms of robustness, clarity, and maintainability.

[amandel]

This pull request implements a button-activated shutdown feature, but introduces several bugs and maintainability issues. The most pressing issues are:

• Incomplete Shutdown: The display blacks out, but the ESP32 doesn't actually power down. ESP.deepSleep() or a similar method needs to be used.
• Potential Race Condition: Calling handleShutdownDisplay() from within the config server handler could cause a race condition. This logic should be moved to the main loop.
• Potential Bug in Shutdown Animation Timing: The linesToFill calculation in displays.cpp assumes BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS is greater than 3000. This assumption needs to be explicitly checked.
• Magic Numbers: Hardcoded timing values like 3000 and 7000 should be replaced with named constants in globals.h.

Recommended Code Changes:

• In displays.cpp:

  • Replace magic numbers with named constants.
  • Add error handling to the linesToFill calculation.
  • Implement actual shutdown using ESP.deepSleep().
  • Rename startedShutdownDisplay to isShutdownInProgress.
  • Change handleShutdownDisplay() return type to void or document its purpose.
  • Remove or refactor the #ifdef OBSPRO block to use configuration instead.

• In globals.h:

  • Add constants SHUTDOWN_DELAY_MS, SHUTDOWN_TOTAL_ANIMATION_MS, and BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS.

• In configServer.cpp:

  • Move the call to handleShutdownDisplay() to the main loop.

• In button.cpp and button.h:

  • Remove unnecessary return values from handle() or document their purpose.
  • Fix the typo in the second handle declaration in button.h.

Additional Recommendations:

• Testing: Add unit tests, especially for the shutdown logic.
• User Feedback: Provide feedback on the display during shutdown (e.g., "Shutting down...").
• Configuration: Allow configuring the shutdown timeout.
• Review the other suggestions in the detailed review, such as improving function names and using enums for state management where appropriate.

Addressing these issues will greatly improve the reliability and maintainability of the shutdown feature.

## Review Summary:

This PR introduces several minor improvements, including adjustments to timing intervals, a clearer shutdown mechanism, and minor code style changes. The changes are generally positive, enhancing functionality and readability. However, there's a potential bug introduced in configServer.cpp and an incomplete implementation. Testing is crucial after merging this PR.

File-Specific Comments:

src/OpenBikeSensorFirmware.cpp:

• Positive Changes:

  • Changing the DISPLAY_INTERVAL_MILLIS from 200 to 300: This might improve performance or battery life, depending on the display's characteristics. However, the reason for this change should be documented in the PR description.
  • Replacing the manual button press counting with the Button class and introducing BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS: This significantly improves the readability and robustness of the shutdown logic. The magic number 50 is replaced with a named constant, and the logic is simplified. This is a good improvement.
  • Using the millis() function only once and storing the value in a local variable (now) within powerKeepAliveTimerISR(): This is a minor optimization that improves code clarity slightly.

• Points to Consider:

  • Clarify the intent behind the DISPLAY_INTERVAL_MILLIS change in the PR description. Why 300ms specifically?
  • Consider adding a comment explaining the purpose of the POWER_KEEP_ALIVE_INTERVAL_MS and its interaction with the IP5306.

src/configServer.cpp:

• Major Issues:

  • Incomplete Implementation: The line obsDisplay->handleShutdownDi in updateDisplay is incomplete and will cause a compilation error. This needs to be completed, presumably as obsDisplay->handleShutdownDisplay();.
  • Potential Bug/Unintended Behavior: Calling obsDisplay->handleShutdownDisplay() within updateDisplay without checking shutdownState might lead to unexpected behavior. The shutdown display logic might be triggered prematurely or unnecessarily. This should be conditional, only called when shutdownState is appropriate. Consider moving the logic to powerKeepAliveTimerISR where the shutdownState is managed.

• Suggestions:

  • Complete the obsDisplay->handleShutdownDi line.
  • Move the call to obsDisplay->handleShutdownDisplay() to a more appropriate location, likely within the switch statement in powerKeepAliveTimerISR where shutdownState is handled.
  • Add a comment explaining the logic of when handleShutdownDisplay() should be called.

.github/fake-cc, .idea/codeStyles/Project.xml, README.md:

• The changes to these files are related to code style and project metadata. The change in .idea/codeStyles/Project.xml simplifies the coding style guidelines, which is generally good. The changes in file modes (100755 to 100644) likely reflect correcting unnecessary execute permissions. These changes are generally acceptable. Make sure the PR description mentions these if they're significant.

General Recommendations:

• Testing: Thoroughly test the shutdown functionality after these changes. Ensure the device shuts down correctly and behaves as expected after the button is held for BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS.
• Documentation: Update any relevant documentation to reflect the changes in timing and shutdown behavior.

By addressing these points, the PR will be significantly improved in terms of functionality, clarity, and maintainability.

Review Summary:

This PR introduces a shutdown functionality triggered by holding down a button. While the functionality seems desirable, the implementation has several issues related to code clarity, potential bugs, and missing configuration handling.

File-Specific Comments:

src/displays.cpp:

• Magic Number: 3000 and 7000 appear multiple times related to the shutdown timing. Replace these with named constants like SHUTDOWN_DELAY_MS and SHUTDOWN_TOTAL_ANIMATION_MS in globals.h (see below) to improve readability and maintainability. This also allows for easily adjusting these timings later if needed. (lines 13-20)
• Missing Error Handling: The calculation linesToFill assumes BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS is greater than 3000. If this isn't the case, the calculation might lead to unexpected behavior. Add a check to ensure this condition is met and handle the error appropriately (e.g., log a warning or cap linesToFill). (line 17)
• Incomplete Shutdown: The code blacks out the display but doesn't actually shut down the device. The PR needs to interface with the power management system of the ESP32 to trigger a proper shutdown. Consider using ESP.deepSleep() or similar. (lines 18-20)
• Unclear Variable Name: startedShutdownDisplay isn't very descriptive. Consider renaming it to something like isShutdownInProgress. (lines 10, 19, 21)
• Unnecessary Return Value: The return value of handleShutdownDisplay is never used. If it's intended for debugging, make that clear in a comment. Otherwise, change the return type to void. (lines 11, 26)
• Preprocessor Directive Misplaced? The #ifdef OBSPRO block suggests this feature is conditional. It's better to handle feature flags through configuration rather than preprocessor directives. This improves testability and makes it easier to enable/disable features without recompilation. (line 12)

src/displays.h:

• Comment Style Inconsistency: The comment style for handleShutdownDisplay doesn't match the rest of the file. Maintain consistency throughout. (line 100)

src/globals.h:

• Add Constants: Add the following constants for the shutdown timing values and the button press threshold:

extern const uint16_t SHUTDOWN_DELAY_MS;  // 3000
extern const uint16_t SHUTDOWN_TOTAL_ANIMATION_MS; // Based on calculation, ensure it's > SHUTDOWN_DELAY_MS
extern const uint16_t BUTTON_PRESS_THRESHOLD_FOR_SHUTDOWN_MS; // 7000

src/utils/button.cpp:

• Unnecessary Return Value: Similar to handleShutdownDisplay, the return value from handle() appears unused. Remove it or document its purpose. If button state needs to be checked externally, provide a separate getState() method. (lines 31, 35, 52)

src/utils/button.h:

• Typo: There's a typo at the end of the file. The second handle declaration is incomplete. (line 29)

src/configserver.cpp:

• Potential Race Condition: Calling obsDisplay->handleShutdownDisplay() within configServerHandle() could lead to a race condition if the button is held while the config server is being accessed. The shutdown logic shouldn't be tied to the config server handling. Consider moving obsDisplay->handleShutdownDisplay() to the main loop, so it's checked independently and frequently. (line 2069)

General Suggestions:

• Testing: Add unit tests to verify the shutdown logic, including the animation and the interaction with the power management system.
• User Feedback: Provide visual feedback to the user during the shutdown process (e.g., a "Shutting down..." message on the display).
• Configuration: Allow the user to configure the shutdown timeout and whether the feature is enabled at all.

By addressing these points, the code will be more robust, maintainable, and user-friendly.

File: (Not provided, please provide the filename for accurate referencing)

Review Summary: The provided code snippet shows the declaration of a class member functions related to time and state management. While the snippet itself doesn't have glaring errors, there are potential issues and areas for improvement depending on the intended usage and surrounding code.

Issues and Suggestions:

1. unsigned long millis() Ambiguity:

   • Issue: This looks like an attempt to shadow the Arduino millis() function. Shadowing built-in functions is generally a bad practice as it can lead to confusion and unexpected behavior.
   • Suggestion: Rename this function to something more descriptive and specific to its purpose, e.g., getMillis(), getLastUpdateTime(), getTimeStamp(), etc. This clarifies its role and avoids conflicts.

2. Missing Return Type for gotPressed():

   • Issue: In modern C++, it's best practice to explicitly specify the return type, even if it's bool. Relying on implicit return types can reduce code clarity.
   • Suggestion: Add bool as the return type: bool gotPressed();

3. int read() Ambiguity:

   • Issue: The function name read() is very generic. Without context, it's hard to determine its purpose. This can make the code difficult to understand and maintain.
   • Suggestion: Use a more descriptive name that reflects what is being read, e.g., readSensorValue(), readButtonState(), readConfiguration(), etc.

4. int getState() Possible Inefficiency (minor):

   • Issue: Returning int for a state might be overkill if the state is simple (e.g., on/off, high/low). Using smaller data types like bool or enum can improve efficiency, particularly if the state is accessed frequently.
   • Suggestion: Consider using bool or an enum if the state represents a limited set of values. This improves readability and can optimize memory usage. For example:

     enum class State { Off, On };
     State getState() const;

5. Missing const for millis() (if intended as a getter):

   • Issue: If the intention of millis() is to retrieve a timestamp, it should be marked as const as it doesn't modify the object's state.
   • Suggestion: Add const: unsigned long millis() const; (or the suggested better name).

6. Potential for Race Conditions (if used with interrupts):

   • Issue: If getState() or gotPressed() are called from within an interrupt service routine (ISR) and the same variables they access are modified in the main loop, you could have race conditions leading to unexpected behavior.
   • Suggestion: If used with interrupts, protect shared variables with appropriate synchronization mechanisms like mutexes or critical sections.

Please provide the full file context (or at least the class definition) for a more thorough review. Also, explaining the intended purpose of each function will help in providing more tailored suggestions.

</details>

[gluap]

Playing with copilot?