mirror of
https://github.com/mikecarper/MeshCore.git
synced 2026-07-27 15:01:06 +00:00
agessaman
98f176c730
- Remove blocking delays in main loop (health check, WiFi reconnect) - Make WiFi initialization fully async and non-blocking - Optimize token renewal to only disconnect when token expires (60s buffer) - Fix health check connection state management - Add comprehensive stability review documentation
119 lines
5.0 KiB
Markdown
119 lines
5.0 KiB
Markdown
# MQTT Connection Stability & CLI Responsiveness Review
|
|
|
|
## Summary
|
|
|
|
This document identifies issues in the MQTT codebase that could cause connection instability or unresponsive CLI behavior.
|
|
|
|
## Critical Issues Fixed
|
|
|
|
### 1. **Blocking Delays in Main Loop** ✅ FIXED
|
|
- **Location**: `connectToBrokers()` line 743, `loop()` line 475
|
|
- **Issue**: `delay(100)` calls block the entire system for 100ms during:
|
|
- Health check reconnections (every 4 hours)
|
|
- WiFi reconnection attempts
|
|
- **Impact**:
|
|
- Blocks CLI responsiveness
|
|
- Delays mesh radio operations
|
|
- Causes system-wide pauses
|
|
- **Fix**: Removed blocking delays - `disconnect()` and `connect()` are async operations
|
|
|
|
### 2. **Blocking WiFi Initialization** ✅ FIXED
|
|
- **Location**: `begin()` line 215-219
|
|
- **Issue**: Blocking `delay(500)` loop for up to 10 seconds (20 attempts) during WiFi initialization
|
|
- **Impact**:
|
|
- Blocks system startup
|
|
- Delays mesh radio initialization
|
|
- CLI unresponsive during startup
|
|
- **Fix**: Removed blocking wait - WiFi connection is now fully asynchronous
|
|
|
|
### 3. **Token Renewal Disconnection Strategy** ✅ FIXED (Previous Change)
|
|
- **Location**: `maintainAnalyzerConnections()` lines 1947-1958, 2052-2063
|
|
- **Issue**: Tokens were renewed 1 hour before expiration, causing unnecessary disconnections
|
|
- **Impact**: Connection drops every 23 hours
|
|
- **Fix**: Changed to renew only 60 seconds before expiration, only disconnect when token is actually expired
|
|
|
|
## Remaining Issues & Recommendations
|
|
|
|
### 4. **Static Variable Sharing in Health Check** ⚠️ MINOR
|
|
- **Location**: `connectToBrokers()` line 727
|
|
- **Issue**: `static unsigned long last_health_check` is shared across all brokers
|
|
- **Impact**: If multi-broker support is added, all brokers will trigger health checks simultaneously
|
|
- **Recommendation**: Use per-broker health check timestamps (store in broker struct)
|
|
- **Priority**: Low (current code only supports one broker at a time)
|
|
|
|
### 5. **NTP Sync Blocking** ⚠️ ACCEPTABLE
|
|
- **Location**: `syncTimeWithNTP()` line 2178
|
|
- **Issue**: `forceUpdate()` is a blocking call that can take several seconds
|
|
- **Impact**: Blocks main loop during NTP sync (every hour)
|
|
- **Status**: Acceptable - only happens once per hour, has internal timeout
|
|
- **Recommendation**: Consider making NTP sync fully async if responsiveness becomes an issue
|
|
|
|
### 6. **vTaskDelay in Packet Processing** ⚠️ ACCEPTABLE
|
|
- **Location**: `processPacketQueue()` line 833
|
|
- **Issue**: `vTaskDelay(1)` yields for ~10ms on ESP32 after each packet
|
|
- **Impact**: Adds latency to packet processing
|
|
- **Status**: Acceptable - prevents blocking, maintains responsiveness
|
|
- **Note**: This is intentional to yield to other tasks
|
|
|
|
### 7. **Connection State Race Conditions** ⚠️ POTENTIAL
|
|
- **Location**: Multiple locations checking `connected()` then using connection
|
|
- **Issue**: Connection state could change between check and use
|
|
- **Impact**: Potential for failed publishes or incorrect state tracking
|
|
- **Status**: Mitigated by async nature of PsychicMqttClient
|
|
- **Recommendation**: Monitor for any issues, consider adding connection state locks if problems occur
|
|
|
|
### 8. **Health Check Connection State Management** ✅ FIXED
|
|
- **Location**: `connectToBrokers()` line 754-756
|
|
- **Issue**: Health check reconnection didn't properly mark broker as disconnected
|
|
- **Impact**: Could cause state inconsistency
|
|
- **Fix**: Now properly marks broker as disconnected before async reconnection
|
|
|
|
## Performance Characteristics
|
|
|
|
### Before Fixes
|
|
- **WiFi initialization**: Up to 10 seconds blocking
|
|
- **Health check**: 100ms blocking every 4 hours
|
|
- **WiFi reconnect**: 100ms blocking during reconnect attempts
|
|
- **Token renewal**: Disconnection every 23 hours (1 hour before expiration)
|
|
|
|
### After Fixes
|
|
- **WiFi initialization**: Fully async, non-blocking
|
|
- **Health check**: Fully async, non-blocking
|
|
- **WiFi reconnect**: Fully async, non-blocking
|
|
- **Token renewal**: Disconnection only when token expires (within 60 seconds)
|
|
|
|
## Testing Recommendations
|
|
|
|
1. **Long-term stability test**: Run for 24+ hours and monitor:
|
|
- Connection uptime
|
|
- Queue length spikes
|
|
- Status publishing gaps
|
|
- CLI responsiveness
|
|
|
|
2. **Stress test**:
|
|
- Rapid WiFi disconnections/reconnections
|
|
- High packet rate
|
|
- Memory pressure scenarios
|
|
|
|
3. **CLI responsiveness test**:
|
|
- Issue CLI commands during:
|
|
- Token renewal
|
|
- Health check reconnection
|
|
- High packet processing
|
|
- WiFi reconnection
|
|
|
|
## Code Quality Notes
|
|
|
|
- All blocking operations have been removed from the main loop
|
|
- Connection operations are now fully asynchronous
|
|
- Token renewal strategy optimized to minimize disconnections
|
|
- Health check properly manages connection state
|
|
|
|
## Future Improvements
|
|
|
|
1. **Per-broker health check timestamps**: Store in broker struct instead of static variable
|
|
2. **Async NTP sync**: Consider making NTP sync fully async if responsiveness issues occur
|
|
3. **Connection state locking**: Add locks if race conditions become an issue
|
|
4. **Multi-broker support**: Implement proper multi-broker support with per-broker state management
|
|
|