do not reboot during health check #49
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @tavdog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily prevents the system from automatically rebooting when WiFi connectivity is lost for an extended period during health checks. It also refines the application's network error handling to be more explicit about WiFi status and updates related logging messages.
Highlights
- WiFi Health Check: Removed the automatic system reboot mechanism that was previously triggered after a threshold of consecutive WiFi disconnections during health checks.
- Application Network Handling: Improved error handling in the main application loop by explicitly checking for WiFi connectivity before attempting remote data retrieval, providing a more accurate log message if WiFi is not connected.
- Logging Improvements: Updated log messages within the WiFi health check function to remove the hardcoded disconnect threshold and rephrase reconnect messages for better clarity.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request disables the system reboot functionality that was previously triggered by multiple consecutive WiFi disconnection events during the health check. The changes primarily involve:
- In
src/wifi.c:- Commenting out the code block responsible for
esp_restart()after a set number of failed health checks. - Updating a related log message to reflect that the reboot threshold is no longer active.
- Commenting out the code block responsible for
- In
src/main.c:- Adding a check to ensure WiFi is connected before attempting to fetch data via
remote_get(). This is a good addition as it prevents unnecessary network calls when WiFi is known to be down.
- Adding a check to ensure WiFi is connected before attempting to fetch data via
The changes effectively address the goal stated in the PR title. My review includes suggestions to improve code clarity and maintainability by addressing the commented-out code and a log message format.
Additionally, please note that the documentation for wifi_health_check() in src/wifi.h (specifically lines 77-78) should be updated. It currently states that the system will reboot after 10 consecutive failures, which is no longer the case.
| @@ -783,11 +783,11 @@ void wifi_health_check(void) { | |||
|
|
|||
| // WiFi is not connected, increment counter | |||
| s_wifi_disconnect_counter++; | |||
| ESP_LOGW(TAG, "WiFi Health check. Disconnect count: %d/15", s_wifi_disconnect_counter); | |||
| ESP_LOGW(TAG, "WiFi Health check. Disconnect count: %d/*", s_wifi_disconnect_counter); | |||
There was a problem hiding this comment.
The log message format Disconnect count: %d/* is a bit unconventional and might be unclear to others reading the logs. The /* seems to indicate that the previous threshold (e.g., /15) for rebooting is no longer applicable. It would be more explicit to either remove this part or clarify the meaning for better log readability.
ESP_LOGW(TAG, "WiFi Health check. Disconnect count: %d (reboot on disconnect disabled)", s_wifi_disconnect_counter);| // if (s_wifi_disconnect_counter >= 15) { | ||
| // ESP_LOGE(TAG, "WiFi disconnected for 15 consecutive checks. Rebooting system..."); | ||
| // // Wait a moment before rebooting | ||
| // vTaskDelay(pdMS_TO_TICKS(1000)); | ||
| // esp_restart(); | ||
| // } |
There was a problem hiding this comment.
This block of code, which handles system reboot after repeated WiFi disconnections, has been commented out. If the decision to remove this reboot functionality is permanent (as implied by the PR title "do not reboot during health check"), it's recommended to delete this commented-out code. Leaving dead code can reduce readability and long-term maintainability.
If there's a possibility this feature might be re-enabled in the future, consider using a compile-time configuration (e.g., a Kconfig option) or a runtime flag instead of commented-out code for better control and clarity.
No description provided.