-
-
Notifications
You must be signed in to change notification settings - Fork 12
prep71 #106
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
prep71 #106
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,9 +36,8 @@ void setDefaults(JsonObject controls, JsonArray definition) { | |
| } | ||
|
|
||
| // shared static variables | ||
| SemaphoreHandle_t ModuleState::updateMutex = xSemaphoreCreateMutex(); | ||
| SemaphoreHandle_t ModuleState::updateMutex = nullptr; | ||
| UpdatedItem ModuleState::updatedItem; | ||
| bool ModuleState::updatePending = false; | ||
| Char<20> ModuleState::updateOriginId; | ||
|
|
||
|
Comment on lines
38
to
42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's examine the file structure and the specific lines mentioned
cd src/MoonBase && wc -l Module.cppRepository: MoonModules/MoonLight Length of output: 79 🏁 Script executed: # Read the problematic code sections
cat -n Module.cpp | sed -n '35,45p'Repository: MoonModules/MoonLight Length of output: 107 🏁 Script executed: # Also check lines 274-279
cat -n Module.cpp | sed -n '270,280p'Repository: MoonModules/MoonLight Length of output: 107 🏁 Script executed: # Let's see the class definition and understand ModuleState
rg "class ModuleState" -A 30Repository: MoonModules/MoonLight Length of output: 1787 🏁 Script executed: # Look for ModuleState::update method
rg "ModuleState::update" -B 2 -A 15Repository: MoonModules/MoonLight Length of output: 25907 🏁 Script executed: # Search for usage of updatedItem and updateOriginId
rg "updatedItem|updateOriginId" -nRepository: MoonModules/MoonLight Length of output: 26488 🏁 Script executed: # Check lines 274-279 to see what else the review comment applies to
head -n 285 src/MoonBase/Module.cpp | tail -n 20Repository: MoonModules/MoonLight Length of output: 866 🏁 Script executed: # Look at the full postUpdate implementation to understand synchronization
rg "void postUpdate" -B 2 -A 20Repository: MoonModules/MoonLight Length of output: 1524 🏁 Script executed: # Check for concurrent access patterns to updateOriginId
rg "updateOriginId" -B 1 -A 1 | head -100Repository: MoonModules/MoonLight Length of output: 2904 Static state members should be synchronized or converted to instance variables to prevent data races across concurrent module updates. The static Consider making these instance members rather than static to eliminate the shared mutable state:
This also applies to lines 274–316, where the 🤖 Prompt for AI Agents |
||
| void ModuleState::setupData() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,16 +46,18 @@ class ModuleState { | |
|
|
||
| static UpdatedItem updatedItem; | ||
| static SemaphoreHandle_t updateMutex; | ||
| static bool updatePending; | ||
| volatile bool updatePending = false; | ||
|
|
||
| static Char<20> updateOriginId; // static, written by ModuleState::update, no mutex needed as written by one process at a time (http mostly, sveltekit sometimes recursively) | ||
|
|
||
| ModuleState() { | ||
| EXT_LOGD(MB_TAG, "ModuleState constructor"); | ||
|
|
||
| if (updateMutex == nullptr) { | ||
| EXT_LOGE(MB_TAG, "Failed to create updateMutex"); | ||
| } | ||
| if (!updateMutex) { | ||
| EXT_LOGD(MB_TAG, "creating updateMutex"); | ||
| updateMutex = xSemaphoreCreateMutex(); | ||
| if (!updateMutex) EXT_LOGE(MB_TAG, "Failed to create updateMutex"); | ||
| } | ||
|
Comment on lines
47
to
60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical: static Scenario: Module A sets Strongly suggest making the queued state per
Proposed direction (per-instance storage)- static UpdatedItem updatedItem;
- static SemaphoreHandle_t updateMutex;
- volatile bool updatePending = false;
- static Char<20> updateOriginId;
+ UpdatedItem updatedItem;
+ SemaphoreHandle_t updateMutex = nullptr;
+ bool updatePending = false;
+ Char<20> updateOriginId;(And update corresponding definitions/uses in Also applies to: 112-141
Comment on lines
53
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Find and examine Module.h to understand updateMutex declaration and ModuleState structure
fd -t f "Module.h" --exec cat -n {} +Repository: MoonModules/MoonLight Length of output: 14144 🏁 Script executed: # Search for ModuleState class definition and updateMutex declaration
rg -A 20 "class ModuleState" src/MoonBase/Repository: MoonModules/MoonLight Length of output: 1225 🏁 Script executed: # Check how many ModuleState instances are created and in what contexts
rg "ModuleState" src/ -t cpp -t h --context 2Repository: MoonModules/MoonLight Length of output: 21114 🏁 Script executed: # Compare with PhysicalLayer and Nodes.h to understand the project's mutex pattern
fd -t f "PhysicalLayer.cpp" "Nodes.h" --exec echo "File: {}" \; -exec head -100 {} \;Repository: MoonModules/MoonLight Length of output: 307 🏁 Script executed: # Search for Module instantiation/creation patterns
rg "new Module|Module(" src/ -t cpp -t h --context 2 | head -100Repository: MoonModules/MoonLight Length of output: 151 🏁 Script executed: # Check Module.cpp constructor to understand instantiation context
cat src/MoonBase/Module.cpp | head -150Repository: MoonModules/MoonLight Length of output: 6645 🏁 Script executed: # Compare updateMutex pattern with other mutexes in project (effectsMutex, driversMutex, etc.)
rg "xSemaphoreCreateMutex" src/ -t cpp -t h -B 3 -A 3Repository: MoonModules/MoonLight Length of output: 1316 🏁 Script executed: # Check main.cpp to see how Module instances are created
rg "Module" src/main.cpp -A 2 -B 2 | head -80Repository: MoonModules/MoonLight Length of output: 3210 🏁 Script executed: # Look for any dynamic Module instantiation or creation from tasks
rg "new Module|new ModuleChannels|new ModuleLights" src/ -t cpp -t hRepository: MoonModules/MoonLight Length of output: 47 🏁 Script executed: # Compare PhysicalLayer mutex initialization style with ModuleState
rg "SemaphoreHandle_t.*xSemaphoreCreateMutex" src/ -B 2 -A 2Repository: MoonModules/MoonLight Length of output: 727 Mutex initialization pattern differs from project conventions; consider aligning with PhysicalLayer's immediate creation approach. The lazy initialization of Also, 🤖 Prompt for AI Agents |
||
|
|
||
| if (!gModulesDoc) { | ||
| EXT_LOGD(MB_TAG, "Creating doc"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -604,14 +604,13 @@ class ModuleIO : public Module { | |
| } | ||
| } | ||
|
|
||
|
|
||
| void readPins() { | ||
| uint8_t pinRS485TX = UINT8_MAX; | ||
| uint8_t pinRS485RX = UINT8_MAX; | ||
| uint8_t pinRS485DE = UINT8_MAX; | ||
| uint8_t pinRS485TX = UINT8_MAX; | ||
| uint8_t pinRS485RX = UINT8_MAX; | ||
| uint8_t pinRS485DE = UINT8_MAX; | ||
|
|
||
| #if FT_ENABLED(FT_ETHERNET) | ||
| EXT_LOGD(MB_TAG, "Try to configure ethernet"); | ||
| EXT_LOGI(MB_TAG, "Try to configure ethernet"); | ||
| EthernetSettingsService* ess = _sveltekit->getEthernetSettingsService(); | ||
| #ifdef CONFIG_IDF_TARGET_ESP32S3 | ||
| // Note: Ethernet pin types are signed (int8_t) and use -1, not UINT8_MAX, to indicate unset state | ||
|
|
@@ -672,7 +671,7 @@ class ModuleIO : public Module { | |
| } else if (usage == pin_Battery) { | ||
| pinBattery = pinObject["GPIO"]; | ||
| EXT_LOGD(ML_TAG, "pinBattery found %d", pinBattery); | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
|
|
||
|
|
@@ -703,7 +702,7 @@ class ModuleIO : public Module { | |
| } else if (usage == pin_RS485_TX) { | ||
| rs485_ios_updated = true; | ||
| pinRS485TX = pinObject["GPIO"]; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check if all RS485 pins are specified | ||
|
|
@@ -714,9 +713,9 @@ class ModuleIO : public Module { | |
| uart_config_t uart_config = { | ||
| .baud_rate = 9600, | ||
| .data_bits = UART_DATA_8_BITS, | ||
| .parity = UART_PARITY_DISABLE, | ||
| .parity = UART_PARITY_DISABLE, | ||
| .stop_bits = UART_STOP_BITS_1, | ||
| .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, // Flow control handled by RS485 driver | ||
| .flow_ctrl = UART_HW_FLOWCTRL_DISABLE, // Flow control handled by RS485 driver | ||
| .source_clk = UART_SCLK_DEFAULT, | ||
| }; | ||
| uart_driver_delete(UART_NUM_1); | ||
|
|
@@ -725,7 +724,7 @@ class ModuleIO : public Module { | |
| ESP_ERROR_CHECK(uart_set_pin(UART_NUM_1, pinRS485TX, pinRS485RX, pinRS485DE, UART_PIN_NO_CHANGE)); | ||
| ESP_ERROR_CHECK(uart_set_mode(UART_NUM_1, UART_MODE_RS485_HALF_DUPLEX)); | ||
|
|
||
| #ifdef DEMOCODE_FOR_SHT30_SENSOR | ||
| #ifdef DEMOCODE_FOR_SHT30_SENSOR | ||
| // Modbus RTU Request: [Addr][Func][RegHi][RegLo][CountHi][CountLo][CRC_L][CRC_H] | ||
| // To read Reg 0 & 1 from Slave 0x01: 01 03 00 00 00 02 C4 0B | ||
| const uint8_t request[] = {0x01, 0x03, 0x00, 0x00, 0x00, 0x02, 0xC4, 0x0B}; | ||
|
|
@@ -736,19 +735,19 @@ class ModuleIO : public Module { | |
|
|
||
| // Wait for response (timeout 1 second) | ||
| int len = uart_read_bytes(UART_NUM_1, response, 128, pdMS_TO_TICKS(100)); | ||
|
|
||
| if (len > 8) { | ||
| EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); | ||
| float humidity = ((float)response[3])*256 + (float)response[4]; | ||
| float temperature = ((float)response[5])*256 +(float)response[6]; | ||
| EXT_LOGD(ML_TAG, "humidity: %f temperature: %f", humidity/10, temperature/10); | ||
| // Process registers here (response[3] to response[6] contain the data) | ||
| EXT_LOGD(ML_TAG, "Answer received: %d %d %d %d %d %d %d %d %d", response[0], response[1], response[2], response[3], response[4], response[5], response[6], response[7], response[8]); | ||
| float humidity = ((float)response[3]) * 256 + (float)response[4]; | ||
| float temperature = ((float)response[5]) * 256 + (float)response[6]; | ||
| EXT_LOGD(ML_TAG, "humidity: %f temperature: %f", humidity / 10, temperature / 10); | ||
| // Process registers here (response[3] to response[6] contain the data) | ||
| } else if (len > 0) { | ||
| EXT_LOGD(ML_TAG, "Invalid answer length"); | ||
| EXT_LOGD(ML_TAG, "Invalid answer length"); | ||
| } else { | ||
| EXT_LOGD(ML_TAG, "No response from sensor"); | ||
| } | ||
| #endif | ||
| EXT_LOGD(ML_TAG, "No response from sensor"); | ||
| } | ||
| #endif | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -796,8 +795,12 @@ class ModuleIO : public Module { | |
| uint32_t adc_mv_vinput = analogReadMilliVolts(pinVoltage); | ||
| analogSetAttenuation(ADC_11db); | ||
| float volts = 0; | ||
| if (current_board_id == board_SE16V1) { volts = ((float)adc_mv_vinput) * 2 / 1000; } // /2 resistor divider | ||
| else if (current_board_id == board_LightCrafter16) { volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); } // 1k43/10k resistor divider | ||
| if (current_board_id == board_SE16V1) { | ||
| volts = ((float)adc_mv_vinput) * 2 / 1000; | ||
| } // /2 resistor divider | ||
| else if (current_board_id == board_LightCrafter16) { | ||
| volts = ((float)adc_mv_vinput) * 11.43 / (1.43 * 1000); | ||
| } // 1k43/10k resistor divider | ||
| batteryService->updateVoltage(volts); | ||
| voltage_readout_current_adc_attenuation = adc_get_adjusted_gain(voltage_readout_current_adc_attenuation, adc_mv_vinput); | ||
| } | ||
|
Comment on lines
+798
to
806
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Battery voltage: avoid pushing Right now
🤖 Prompt for AI Agents |
||
|
|
@@ -809,8 +812,12 @@ class ModuleIO : public Module { | |
| if ((current_board_id == board_SE16V1) || (current_board_id == board_LightCrafter16)) { | ||
| if (adc_mv_cinput > 330) // datasheet quiescent output voltage of 0.5V, which is ~330mV after the 10k/5k1 voltage divider. Ideally, this value should be measured at boot when nothing is displayed on the LEDs | ||
| { | ||
| if (current_board_id == board_SE16V1) { batteryService->updateCurrent((((float)(adc_mv_cinput)-250) * 50.00) / 1000); } // 40mV / A with a /2 resistor divider, so a 50mA/mV | ||
| else if (current_board_id == board_LightCrafter16) { batteryService->updateCurrent((((float)(adc_mv_cinput)-330) * 37.75) / 1000); } // 40mV / A with a 10k/5k1 resistor divider, so a 37.75mA/mV | ||
| if (current_board_id == board_SE16V1) { | ||
| batteryService->updateCurrent((((float)(adc_mv_cinput)-250) * 50.00) / 1000); | ||
| } // 40mV / A with a /2 resistor divider, so a 50mA/mV | ||
| else if (current_board_id == board_LightCrafter16) { | ||
| batteryService->updateCurrent((((float)(adc_mv_cinput)-330) * 37.75) / 1000); | ||
| } // 40mV / A with a 10k/5k1 resistor divider, so a 37.75mA/mV | ||
| } else { | ||
| batteryService->updateCurrent(0); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.