-
Notifications
You must be signed in to change notification settings - Fork 0
Require SD card configuration for both server and client settings (remove fallback defaults) #40
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
Conversation
Co-authored-by: dorkmo <[email protected]>
|
@copilot let's YOLO it and remove |
Co-authored-by: dorkmo <[email protected]>
Done! Removed the graceful fallback behavior. The server now requires SD card configuration and will halt with error messages if:
Changes in commit
The YOLO approach is now in effect - no safety net, SD card configuration must be present and valid! 🚀 |
|
@copilot can you now make the client operate the same way as the changes made to the server. We can come back and change how the sensors pins are reserved later. |
Co-authored-by: dorkmo <[email protected]>
Done! Applied the same SD card requirement changes to the client. The client now operates exactly like the server changes: Client Changes in commit
Client will now HALT with error messages if:
Sensor pins kept as compile-time constants as requested - all the hardware-specific pin definitions, sensor types, and I2C addresses remain in Both server and client now have the same "YOLO" approach - no safety net, SD card configuration must be present and valid! 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements mandatory SD card-based configuration for both Tank Alarm Server and Client, removing all fallback defaults to ensure consistent field-configurable operation. The change enables configuration updates without code recompilation by requiring SD card files at startup.
- Migrated all configuration from compile-time constants to runtime SD card files
- Added mandatory configuration validation with device halt on missing/invalid settings
- Updated documentation to reflect the new "SD card required" approach
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server_config.txt | New SD card configuration template for server with all required settings |
| server_config.h | Stripped down to hardware constants only, removed fallback configuration values |
| TankAlarm-092025-Server-Hologram.ino | Added loadServerConfigurationFromSD() function and mandatory validation logic |
| SD_CARD_CONFIG.md | New documentation for required SD card configuration process |
| INSTALLATION.md (Server) | Updated to emphasize mandatory SD card configuration |
| tank_config_example.txt | Updated client configuration template with "REQUIRED" emphasis |
| config_template.h | Stripped down to hardware constants only for client |
| TankAlarm-092025-Client-Hologram.ino | Updated loadSDCardConfiguration() to require SD card with validation |
| INSTALLATION.md (Client) | Updated to reflect mandatory SD card configuration approach |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| unsigned long lastDailyEmailRetry = 0; | ||
| const unsigned long DAILY_EMAIL_RETRY_INTERVAL = 1800000; // 30 minutes in milliseconds | ||
| const int MAX_TANK_REPORTS = 50; // Maximum number of reports to store in memory | ||
| const int MAX_TANK_REPORTS = 100; // Maximum array size (actual limit from configuration) |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment suggests this is the maximum array size but the actual limit comes from configuration. This could be confusing since the array is statically allocated at 100 but maxReportsInMemory can be configured to a different value. Consider renaming to clarify this is the hard limit: // Hard array limit (configured limit must not exceed this)
| const int MAX_TANK_REPORTS = 100; // Maximum array size (actual limit from configuration) | |
| const int MAX_TANK_REPORTS = 100; // Hard array limit (configured limit must not exceed this) |
|
|
||
| // Store report in memory | ||
| if (reportCount < MAX_TANK_REPORTS) { | ||
| if (reportCount < maxReportsInMemory && reportCount < MAX_TANK_REPORTS) { |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition checks both maxReportsInMemory and MAX_TANK_REPORTS but doesn't validate that maxReportsInMemory <= MAX_TANK_REPORTS during configuration loading. If maxReportsInMemory is set higher than MAX_TANK_REPORTS in the config file, this will silently cap at the lower value without warning the user.
| int pos = 0; | ||
| for (int i = 0; i < 4; i++) { | ||
| int nextPos = value.indexOf(',', pos); | ||
| if (nextPos == -1 && i < 3) break; | ||
| String octet = (nextPos == -1) ? value.substring(pos) : value.substring(pos, nextPos); | ||
| staticIpAddress[i] = octet.toInt(); | ||
| pos = nextPos + 1; | ||
| } | ||
| } else if (key == "STATIC_GATEWAY") { | ||
| // Parse comma-separated gateway address | ||
| int pos = 0; | ||
| for (int i = 0; i < 4; i++) { | ||
| int nextPos = value.indexOf(',', pos); | ||
| if (nextPos == -1 && i < 3) break; | ||
| String octet = (nextPos == -1) ? value.substring(pos) : value.substring(pos, nextPos); | ||
| staticGateway[i] = octet.toInt(); | ||
| pos = nextPos + 1; | ||
| } | ||
| } else if (key == "STATIC_SUBNET") { | ||
| // Parse comma-separated subnet mask | ||
| int pos = 0; | ||
| for (int i = 0; i < 4; i++) { | ||
| int nextPos = value.indexOf(',', pos); | ||
| if (nextPos == -1 && i < 3) break; | ||
| String octet = (nextPos == -1) ? value.substring(pos) : value.substring(pos, nextPos); | ||
| staticSubnet[i] = octet.toInt(); | ||
| pos = nextPos + 1; | ||
| } |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This IP address parsing logic is duplicated three times (for static IP, gateway, and subnet). Consider extracting this into a helper function like parseCommaSeparatedIP(String value, int* targetArray) to reduce code duplication and make it easier to maintain.
| int pos = 0; | |
| for (int i = 0; i < 4; i++) { | |
| int nextPos = value.indexOf(',', pos); | |
| if (nextPos == -1 && i < 3) break; | |
| String octet = (nextPos == -1) ? value.substring(pos) : value.substring(pos, nextPos); | |
| staticIpAddress[i] = octet.toInt(); | |
| pos = nextPos + 1; | |
| } | |
| } else if (key == "STATIC_GATEWAY") { | |
| // Parse comma-separated gateway address | |
| int pos = 0; | |
| for (int i = 0; i < 4; i++) { | |
| int nextPos = value.indexOf(',', pos); | |
| if (nextPos == -1 && i < 3) break; | |
| String octet = (nextPos == -1) ? value.substring(pos) : value.substring(pos, nextPos); | |
| staticGateway[i] = octet.toInt(); | |
| pos = nextPos + 1; | |
| } | |
| } else if (key == "STATIC_SUBNET") { | |
| // Parse comma-separated subnet mask | |
| int pos = 0; | |
| for (int i = 0; i < 4; i++) { | |
| int nextPos = value.indexOf(',', pos); | |
| if (nextPos == -1 && i < 3) break; | |
| String octet = (nextPos == -1) ? value.substring(pos) : value.substring(pos, nextPos); | |
| staticSubnet[i] = octet.toInt(); | |
| pos = nextPos + 1; | |
| } | |
| parseCommaSeparatedIP(value, staticIpAddress); | |
| } else if (key == "STATIC_GATEWAY") { | |
| // Parse comma-separated gateway address | |
| parseCommaSeparatedIP(value, staticGateway); | |
| } else if (key == "STATIC_SUBNET") { | |
| // Parse comma-separated subnet mask | |
| parseCommaSeparatedIP(value, staticSubnet); |
TankAlarm-092025-Server-Hologram/TankAlarm-092025-Server-Hologram.ino
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| if (alarmPhonePrimary.length() == 0 || alarmPhonePrimary == "+12223334444") { |
Copilot
AI
Sep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded placeholder value +12223334444 should be defined as a constant rather than a magic string. If this placeholder changes in the config template, this validation could miss it. Consider defining #define DEFAULT_PHONE_PLACEHOLDER \"+12223334444\" or validate against the actual template file content.
…ram.ino Co-authored-by: Copilot <[email protected]>
|
@copilot This condition checks both maxReportsInMemory and MAX_TANK_REPORTS but doesn't validate that maxReportsInMemory <= MAX_TANK_REPORTS during configuration loading. If maxReportsInMemory is set higher than MAX_TANK_REPORTS in the config file, this will silently cap at the lower value without warning the user. |
Co-authored-by: dorkmo <[email protected]>
Fixed! Added validation to prevent buffer overflow when Changes in commit
This prevents potential buffer overflow if someone configures a value larger than the allocated array size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (maxReportsInMemory > MAX_TANK_REPORTS) { | ||
| Serial.println("WARNING: MAX_REPORTS_IN_MEMORY (" + String(maxReportsInMemory) + | ||
| ") exceeds MAX_TANK_REPORTS (" + String(MAX_TANK_REPORTS) + ")"); | ||
| Serial.println("Capping maxReportsInMemory to " + String(MAX_TANK_REPORTS)); | ||
| maxReportsInMemory = MAX_TANK_REPORTS; | ||
| } |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The bounds checking logic could be extracted into a separate validation function to improve code organization and reusability, especially since similar validation patterns may be needed for other configuration parameters.
| } else if (key == "STATIC_IP_ADDRESS") { | ||
| // Parse comma-separated IP address like "192,168,1,100" | ||
| int octetCount = 0; | ||
| int lastPos = 0; | ||
| int commaPos = -1; | ||
| byte tempIp[4]; | ||
| bool validIp = true; | ||
| for (int i = 0; i < 4; i++) { | ||
| commaPos = value.indexOf(',', lastPos); | ||
| String octetStr; | ||
| if (i < 3) { | ||
| if (commaPos == -1) { | ||
| validIp = false; | ||
| break; | ||
| } | ||
| octetStr = value.substring(lastPos, commaPos); | ||
| lastPos = commaPos + 1; | ||
| } else { | ||
| octetStr = value.substring(lastPos); | ||
| } | ||
| int octet = octetStr.toInt(); | ||
| if (octet < 0 || octet > 255) { | ||
| validIp = false; | ||
| break; | ||
| } | ||
| tempIp[i] = (byte)octet; | ||
| } | ||
| if (validIp) { | ||
| for (int i = 0; i < 4; i++) staticIpAddress[i] = tempIp[i]; | ||
| } |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IP address parsing logic is duplicated for STATIC_IP_ADDRESS, STATIC_GATEWAY, and STATIC_SUBNET. This code duplication should be extracted into a helper function that returns a parsed IP array to improve maintainability and reduce the risk of inconsistent behavior.
| daysToKeepLogs = value.toInt(); | ||
| } else if (key == "STATIC_IP_ADDRESS") { | ||
| // Parse comma-separated IP address like "192,168,1,100" | ||
| int octetCount = 0; |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable octetCount is declared but never used in the IP address parsing logic. This unused variable should be removed to clean up the code.
| int octetCount = 0; |
|
@copilot The IP address parsing logic is duplicated for STATIC_IP_ADDRESS, STATIC_GATEWAY, and STATIC_SUBNET. This code duplication should be extracted into a helper function that returns a parsed IP array to improve maintainability and reduce the risk of inconsistent behavior. The variable octetCount is declared but never used in the IP address parsing logic. This unused variable should be removed to clean up the code. |
|
@copilot The bounds checking logic could be extracted into a separate validation function to improve code organization and reusability, especially since similar validation patterns may be needed for other configuration parameters. The IP address parsing logic is duplicated for STATIC_IP_ADDRESS, STATIC_GATEWAY, and STATIC_SUBNET. This code duplication should be extracted into a helper function that returns a parsed IP array to improve maintainability and reduce the risk of inconsistent behavior. The variable octetCount is declared but never used in the IP address parsing logic. This unused variable should be removed to clean up the code. |
|
@copilot The IP address parsing logic is duplicated for STATIC_IP_ADDRESS, STATIC_GATEWAY, and STATIC_SUBNET. This code duplication should be extracted into a helper function that returns a parsed IP array to improve maintainability and reduce the risk of inconsistent behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (MONTHLY_REPORT_ENABLED) { | ||
| client.println("<p>Monthly Reports: <span style='color: green;'>Enabled</span> (Day " + String(MONTHLY_REPORT_DAY) + " at " + String(MONTHLY_REPORT_HOUR) + ":00)</p>"); | ||
| client.println("<p>Monthly Reports: <span style='color: green;'>Enabled</span> (Day " + String(monthlyReportDay) + " at " + String(monthlyReportHour) + ":00)</p>"); | ||
| } |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code references MONTHLY_REPORT_ENABLED in the #ifdef directive but uses monthlyReportEnabled variable in the condition check. This creates inconsistent behavior where the compile-time check may not match the runtime configuration.
| byte tempIp[4]; | ||
| bool validIp = true; | ||
| for (int i = 0; i < 4; i++) { | ||
| commaPos = value.indexOf(',', lastPos); | ||
| String octetStr; | ||
| if (i < 3) { | ||
| if (commaPos == -1) { | ||
| validIp = false; | ||
| break; | ||
| } | ||
| octetStr = value.substring(lastPos, commaPos); | ||
| lastPos = commaPos + 1; | ||
| } else { | ||
| octetStr = value.substring(lastPos); | ||
| } | ||
| int octet = octetStr.toInt(); | ||
| if (octet < 0 || octet > 255) { | ||
| validIp = false; | ||
| break; | ||
| } | ||
| tempIp[i] = (byte)octet; | ||
| } |
Copilot
AI
Oct 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IP address parsing logic is duplicated three times for static IP, gateway, and subnet. This should be extracted into a reusable parseIPAddress() helper function to reduce code duplication.
|
@copilot The code references MONTHLY_REPORT_ENABLED in the #ifdef directive but uses monthlyReportEnabled variable in the condition check. This creates inconsistent behavior where the compile-time check may not match the runtime configuration. |
|
@copilot The IP address parsing logic is duplicated three times for static IP, gateway, and subnet. This should be extracted into a reusable parseIPAddress() helper function to reduce code duplication. |
|
@copilot The code references MONTHLY_REPORT_ENABLED in the #ifdef directive but uses monthlyReportEnabled variable in the condition check. This creates inconsistent behavior where the compile-time check may not match the runtime configuration. |
Moving server configuration settings from server_config.h to SD card for field updating capability.
Plan to Move Server Configuration to SD Card
Changes Made
Core Implementation ✅
Configuration Migration ✅
Server - Moved to SD card (server_config.txt) - NOW REQUIRED:
Client - Moved to SD card (tank_config.txt) - NOW REQUIRED:
Kept in config files (essential compile-time constants only):
Field Update Capability ✅
server_config.txttank_config.txtRequired Configuration ✅
Both server and client now assume SD card configuration will always be available and will:
server_config.txtortank_config.txt)Safety Validation ✅
maxReportsInMemoryto prevent buffer overflowMAX_TANK_REPORTS(100), it will be automatically cappedTesting ✅
Documentation ✅
Usage Example
Before (graceful fallback):
After (strict requirement with validation):
Both server and client will HALT with error messages if:
YOLO approach with safety: No safety net for missing config, but automatic bounds checking prevents buffer overflow!
Fixes #39.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.