Fix UART config at OneWire init #3035
Conversation
josesimoes
added
the
Platform: ESP32
WalkthroughThe changes made in this pull request focus on the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)targets/ESP32/_nanoCLR/nanoFramework.Device.OneWire/nf_dev_onewire_nanoFramework_Device_OneWire_OneWireHost.cpp (3)Line range hint The initialization function doesn't need to be in IRAM as it's not called during critical sections or ISRs. This change helps optimize memory usage.
The field-by-field initialization:
Line range hint Since this function is registered as a soft reboot handler and might need to execute while flash is unavailable, removing IRAM_ATTR could potentially cause issues. Let's check if other soft reboot handlers retain IRAM_ATTR: ✅ Verification successfulBased on the gathered information, I can now generate the final response: Removing IRAM_ATTR from oneWireStop is safe The removal of IRAM_ATTR from oneWireStop is acceptable because:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other soft reboot handlers to verify IRAM_ATTR usage pattern
rg -l "HAL_AddSoftRebootHandler" | xargs rg "IRAM_ATTR.*\w+\s*\([^)]*\)\s*{" -B 5 -A 1
Length of output: 210 Script: #!/bin/bash
# First, find files containing soft reboot handlers
rg -l "HAL_AddSoftRebootHandler"
Length of output: 1028 Script: #!/bin/bash
# Then, let's look for IRAM_ATTR usage in ESP32 platform code
rg "IRAM_ATTR" "targets/ESP32"
Length of output: 620 Script: #!/bin/bash
# Let's also check the implementation of HAL_AddSoftRebootHandler
rg -A 5 "HAL_AddSoftRebootHandler\("
Length of output: 8482 Script: #!/bin/bash
# Look for the actual registration of oneWireStop as reboot handler
rg -B 5 -A 5 "HAL_AddSoftRebootHandler.*oneWireStop"
Length of output: 1494 Script: #!/bin/bash
# Let's check the uninitialization handlers in ESP32 platform to see if they use IRAM_ATTR
rg -B 2 "void.*(?i:uninit|stop|cleanup|delete).*\{" "targets/ESP32" --type cpp
Length of output: 311 Script: #!/bin/bash
# Check for any IRAM_ATTR usage in cleanup/stop functions
rg "IRAM_ATTR.*(?i:uninit|stop|cleanup|delete)" "targets/ESP32" --type cpp
Length of output: 74 Script: #!/bin/bash
# Look for soft reboot implementation to understand the context
rg -A 10 "void.*HAL_EnterBooterMode" "targets/ESP32"
Length of output: 52 Script: #!/bin/bash
# Let's look for the soft reboot implementation
rg -A 10 "void HAL_EnterBooterMode|void HAL_SoftReboot" "targets/ESP32"
Length of output: 71 Script: #!/bin/bash
# Check how other cleanup handlers are implemented in ESP32
rg "void.*(?:Stop|Uninitialize|Cleanup).*\(" "targets/ESP32/_nanoCLR" -B 2
Length of output: 3607 Script: #!/bin/bash
# Look for UART driver deletion patterns
rg "uart_driver_delete" "targets/ESP32" -B 5
Length of output: 2049 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- UART config struct is not onlined anymore to set only the required elements. (compiler warning of element not set) - Remove IRAM_ATTR attributes from OneWire API. (no need for there, were left from initial debugging and are only taking up RAM)
josesimoes
force-pushed
the
fix-one-wire-init
branch
from
1c275a9 to
b742bd7
Compare
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit