Fix protecting HB from GC #3062
Conversation
josesimoes
added
Area: Common libs
WalkthroughThe pull request introduces modifications across multiple files in various target platforms, focusing on improving device communication methods for SPI, I2C, and USB interfaces. The changes primarily involve enhancing buffer management, error handling, and resource management for native device communication methods. Modifications include adding buffer pinning for DMA operations, refining transaction handling logic, improving error detection and reporting, and ensuring proper cleanup of resources after device interactions. Changes
Sequence DiagramsequenceDiagram
participant Device
participant NativeMethod
participant DMABuffer
participant ErrorHandler
NativeMethod->>DMABuffer: Pin Buffer
NativeMethod->>Device: Initiate Transaction
alt Successful Transaction
Device-->>NativeMethod: Return Data
NativeMethod->>DMABuffer: Unpin Buffer
else Transaction Error
Device-->>ErrorHandler: Capture Error
ErrorHandler->>NativeMethod: Set Error Status
NativeMethod->>DMABuffer: Unpin Buffer
end
Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
Line range hint
609-610: Handle Potential Null Dereference ofresultAfter creating the
I2cTransferResultobject, there is a null checkFAULT_ON_NULL(result);. However, subsequent code assumesresultis valid. Ensure thatresultis properly initialized and handle the case where memory allocation fails to prevent null pointer dereferences.Apply this diff to handle potential null reference:
CLR_RT_HeapBlock &top = stack.PushValueAndClear(); NANOCLR_CHECK_HRESULT( g_CLR_RT_ExecutionEngine.NewObjectFromIndex(top, g_CLR_RT_WellKnownTypes.m_I2cTransferResult)); result = top.Dereference(); -FAULT_ON_NULL(result); +if (result == NULL) +{ + NANOCLR_SET_AND_LEAVE(CLR_E_OUT_OF_MEMORY); +}🧰 Tools
🪛 cppcheck (2.10-2)
[error] 74-74: Unmatched '{'. Configuration
(syntaxError)
🧹 Nitpick comments (5)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
222-223: Reconsider Timeout Multiplier LogicThe timeout duration is doubled using
estimatedDurationMiliseconds *= 2;. This arbitrary multiplier may not be sufficient for all scenarios, potentially leading to premature timeouts in long transactions. Consider implementing a dynamic scaling factor or allowing configuration of the timeout multiplier to improve reliability.targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
352-372: Consolidate Bus Index ValidationThe bus index validation is spread across multiple switch cases. Consider creating a dedicated function or macro to validate and retrieve the
palI2cinstance based on the bus index. This improves code readability and maintainability.Apply this diff to refactor bus index validation:
NF_PAL_I2C *GetPalI2cInstance(uint8_t busIndex) { switch (busIndex) { case 1: return &I2C1_PAL; case 2: return &I2C2_PAL; // Add cases for other bus indexes default: return NULL; } } // Usage in NativeTransmit method palI2c = GetPalI2cInstance(busIndex); if (palI2c == NULL) { NANOCLR_SET_AND_LEAVE(CLR_E_INVALID_PARAMETER); }targets/TI_SimpleLink/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (2)
180-197: Consider adding documentation for buffer handling logic.While the implementation is robust, adding comments explaining the write buffer handling logic would improve code maintainability.
Add comments like:
// dereference the write and read SpanByte from the arguments writeSpanByte = stack.Arg1().Dereference(); if (writeSpanByte != NULL) { + // Get buffer from SpanByte's underlying array // get buffer writeBuffer = writeSpanByte[SpanByte::FIELD___array].DereferenceArray(); if (writeBuffer != NULL) { + // Calculate actual write size based on span length, not array size // get the size of the buffer by reading the number of elements in the CLR_RT_HeapBlock_Array palI2c->i2cTransaction.writeCount = writeBuffer->m_numOfElements; } }
288-301: Consider additional safety checks in cleanup.While the cleanup properly unpins buffers, consider adding additional safety checks:
if (hr != CLR_E_THREAD_WAITING) { // un-pin the buffers - if (writeBuffer != NULL && writeBuffer->IsPinned()) + if (writeBuffer != NULL) + { + // Double-check if buffer is still valid before unpinning + CLR_RT_HeapBlock* pThis = stack.This(); + if (pThis != NULL && writeBuffer->IsPinned()) + { + writeBuffer->Unpin(); + } + } }targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.UsbStream/sys_dev_usbstream_native_System_Device_Usb_UsbStream.cpp (1)
250-250: Consider documenting long-running operation handling.While the buffer pinning implementation is correct, adding documentation about the long-running operation check would improve code clarity.
Add comments like:
if (isLongRunning && hr != CLR_E_THREAD_WAITING) { + // Only unpin buffer if operation is long-running and not waiting for thread completion // need to clean up the buffer, if this was not rescheduled if (dataBuffer != NULL && dataBuffer->IsPinned()) { dataBuffer->Unpin(); } }Also applies to: 363-374
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp(1 hunks)targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(3 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.UsbStream/sys_dev_usbstream_native_System_Device_Usb_UsbStream.cpp(4 hunks)targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)targets/TI_SimpleLink/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (11)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
284-297: Verify Unpinning of Buffers in All Execution PathsIn the
NANOCLR_CLEANUP()section, buffers are unpinned ifhr != CLR_E_THREAD_WAITING. Ensure that in all possible execution paths, especially in error conditions or exceptions, the buffers are properly unpinned to prevent memory leaks or locked resources.Run the following script to verify that all exit points correctly unpin the buffers:
✅ Verification successful
Buffer Unpinning Implementation is Correct
The cleanup section properly handles buffer unpinning in all execution paths. The implementation:
- Only unpins buffers that were previously pinned (using IsPinned() check)
- Includes NULL checks to prevent invalid operations
- Correctly skips unpinning when thread is waiting (hr == CLR_E_THREAD_WAITING)
- Handles both regular array buffers and SpanByte scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for proper unpinning of writeBuffer and readBuffer in all exit paths. # Search for all return statements and verify buffer unpinning before them. rg 'return' -A 5 src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp | grep -B 5 -A 5 'Unpin'Length of output: 112
Script:
#!/bin/bash # Get the complete function implementation containing NANOCLR_CLEANUP ast-grep --pattern 'NANOCLR_NTSTATUS Library_sys_dev_spi_native_System_Device_Spi_SpiDevice_$$$($$$) { $$$ }' # Search for NANOCLR_CLEANUP macro usage and definition rg "NANOCLR_CLEANUP" -B 5 -A 5 # Look for Pin/Unpin operations rg "Pin|Unpin" -B 2 -A 2Length of output: 75924
Script:
#!/bin/bash # Get the complete implementation of the SPI transfer method ast-grep --pattern 'HRESULT Library_sys_dev_spi_native_System_Device_Spi_SpiDevice_$$$($$$) { $$$ }' # Look for Pin/Unpin operations in the SPI file rg "Pin|Unpin" src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp -B 5 -A 5Length of output: 2967
targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (2)
658-659: Ensure Cache Coherency After DMA Read OperationsAfter the read operation, cache invalidation is performed for Cortex-M7 processors. Ensure that this operation is also performed for other processors where necessary or confirm that it's not required due to hardware coherence.
663-677: Verify Proper Unpinning of BuffersIn the cleanup section, buffers are unpinned if they were pinned earlier. Ensure that this unpinning occurs even if an exception is thrown before the buffers are unpinned. This can prevent memory leaks or other unintended behavior.
Run the following script to check for unpinned buffers in all exit paths:
✅ Verification successful
Buffer Handling is Safe and Correct
The current implementation properly handles buffer pinning and unpinning:
- Buffers are only pinned after thorough validation
- Early exits occur before any pinning operations
- The cleanup section correctly unpins both buffers in all relevant scenarios
- The thread waiting check prevents premature unpinning during async operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that writeBuffer and readBuffer are unpinned in all exit paths. # Search for return or leave statements and check for unpinning rg 'NANOCLR_CLEANUP_END\(\)' -A 10 targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cppLength of output: 160
Script:
#!/bin/bash # Get the complete function implementation ast-grep --pattern 'HRESULT Library_sys_dev_i2c_native_System_Device_I2c_I2cDevice::NativeTransmit$$$' # Look for early exits rg "NANOCLR_SET_AND_LEAVE" targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cppLength of output: 472
Script:
#!/bin/bash # Get the complete function implementation with context rg -B 10 -A 10 "NativeTransmit" targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cppLength of output: 717
Script:
#!/bin/bash # Get the complete function implementation with larger context rg -B 10 -A 100 "HRESULT Library_sys_dev_i2c_native_System_Device_I2c_I2cDevice::NativeTransmit" targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cppLength of output: 192
Script:
#!/bin/bash # Search with shorter unique name part and larger context rg -B 50 -A 100 "NativeTransmit___SystemDeviceI2cI2cTransferResult" targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp # Look specifically for buffer operations rg "buffer.*Pin|Pin.*buffer" targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cppLength of output: 5159
targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
728-732: Ensure Cache Invalidation is Implemented Where NecessaryThe cache invalidation after DMA operations is marked as required only for Cortex-M7 processors. Verify whether other processors (e.g., Cortex-M4, Cortex-M33) with data caches require similar invalidation to maintain data coherence.
Consult the processor documentation or run the following script to check for other processors with caches:
✅ Verification successful
Cache Invalidation Implementation is Correct
The comment "only required for Cortex-M7" is accurate. Among the Cortex-M processors used in the codebase (M0, M4, M7), only the Cortex-M7 has a data cache that requires invalidation after DMA operations. Other processors like Cortex-M4 only have instruction cache, and Cortex-M0 has no cache at all, making this operation unnecessary for them.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Identify processors that have data caches enabled. # List processor definitions that might have data caches rg 'defined\(Cortex-M[4-7]XX\)' targets/AzureRTOS/ST/ -A 5Length of output: 60
Script:
#!/bin/bash # Search for cache-related functions and processor configurations echo "=== Cache-related functions ===" rg "cache.*invalidate|invalidate.*cache" -i echo -e "\n=== Processor configurations ===" rg "CORTEX_M[0-9]" -i echo -e "\n=== Cache management implementations ===" rg "SCB_InvalidateDCache|SCB_CleanDCache" -iLength of output: 19003
targets/TI_SimpleLink/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (3)
143-155: Well-structured variable declarations!The code demonstrates good organization with clear separation of buffer-related variables and proper initialization.
199-215: Consistent read buffer handling implementation!The read buffer handling follows the same robust pattern as write buffer handling, maintaining code consistency.
224-235: Excellent implementation of buffer pinning!The code properly pins both write and read buffers before DMA operations, effectively protecting them from GC relocation. This implementation aligns perfectly with the PR's objective of replacing
CLR_RT_ProtectFromGCwith buffer pinning.targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.UsbStream/sys_dev_usbstream_native_System_Device_Usb_UsbStream.cpp (1)
108-108: Proper buffer pinning implementation in Read method!The code correctly pins the buffer before DMA operations and implements proper cleanup with thread state verification.
Also applies to: 203-214
targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (3)
475-477: Well-documented write buffer pinning!The code properly pins the write buffer for DMA operations with clear comments explaining the purpose.
501-503: Consistent read buffer pinning implementation!The read buffer pinning follows the same well-documented pattern as write buffer pinning.
731-748: Robust cleanup implementation!The cleanup properly handles both write and read buffers, with appropriate thread state verification before unpinning.
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp
Outdated
Show resolved
Hide resolved
...s/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp
Outdated
Show resolved
Hide resolved
...s/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp
Show resolved
Hide resolved
- Replaced CLR_RT_ProtectFromGC with pinning buffer HB. - Pinned buffer HB where missing it.
josesimoes
force-pushed
the
pinning-data-buffers
branch
from
358f99d to
87b3308
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
189-250: Consider adjusting the timeout calculation for edge cases.The timeout is set to twice the estimated duration, but this might not be sufficient for all scenarios, especially in systems under heavy load or with slower clock speeds.
Consider using a larger multiplier or adding a minimum timeout value:
-estimatedDurationMiliseconds *= 2; +// Use 3x estimated time or minimum 100ms, whichever is larger +estimatedDurationMiliseconds = MAX(estimatedDurationMiliseconds * 3, 100);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp(1 hunks)targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(6 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.UsbStream/sys_dev_usbstream_native_System_Device_Usb_UsbStream.cpp(4 hunks)targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)targets/TI_SimpleLink/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.UsbStream/sys_dev_usbstream_native_System_Device_Usb_UsbStream.cpp
- targets/TI_SimpleLink/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp
🧰 Additional context used
📓 Learnings (2)
targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3062
File: targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp:393-410
Timestamp: 2025-01-09T13:18:50.055Z
Learning: In nanoFramework's I2C implementation for STM32 targets, the palI2c pointer is always assigned to a static PAL instance (I2C1_PAL, I2C2_PAL, etc.) through an exhaustive switch statement. Invalid bus indices are caught by the default case, making NULL checks on palI2c unnecessary after the switch statement.
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:77-93
Timestamp: 2025-01-09T13:10:31.388Z
Learning: In nanoFramework SPI transactions, NULL buffers are valid cases when there is no data to send or receive. The code should handle NULL buffers by setting corresponding sizes to 0 instead of throwing exceptions.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (5)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (2)
77-93: Well-structured variable declarations!The code follows good practices with proper initialization of pointers and appropriate type usage for different variables.
286-297: Robust cleanup implementation!The cleanup logic properly handles buffer unpinning and checks the thread state before performing cleanup operations.
targets/ChibiOS/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
Line range hint
1-753: Implementation follows established patterns.The changes in this file mirror the improvements made in the SPI implementation, including buffer pinning, error handling, and resource cleanup.
targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
Line range hint
1-749: Platform-specific implementation follows established patterns.The changes align with the improvements made in other I2C implementations, adapted for the SiliconLabs platform.
targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
392-753: Platform-specific implementation follows established patterns.The changes align with the improvements made in other I2C implementations, adapted for the ST platform.
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp
Outdated
Show resolved
Hide resolved
Automated fixes for code style.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
106-134: Add error handling for buffer operations.While the buffer pinning implementation is correct, consider adding error handling for:
- GetFirstElementUInt16() operations
- Memory allocation for large buffers
Apply this diff to add error handling:
if (writeBuffer != NULL) { + // Check if buffer allocation is successful + FAULT_ON_NULL(writeBuffer->GetFirstElementUInt16()); writeData = (unsigned char *)writeBuffer->GetFirstElementUInt16(); writeSize = writeBuffer->m_numOfElements; writeBuffer->Pin(); }targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
461-512: Add documentation for DMA requirements.While the buffer pinning implementation is correct, it would be helpful to add a comment explaining why pinning is required for DMA operations.
Add this comment before pinning operations:
+ // Pin buffers to prevent GC from moving them during DMA operations writeBuffer->Pin();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp(1 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:77-93
Timestamp: 2025-01-09T13:10:31.388Z
Learning: In nanoFramework SPI transactions, NULL buffers are valid cases when there is no data to send or receive. The code should handle NULL buffers by setting corresponding sizes to 0 instead of throwing exceptions.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (4)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (3)
77-94: Well-structured variable declarations!Good organization of variables with clear separation between SpanByte and Array handling. Proper initialization of pointers to NULL and sizes to 0.
135-188: Correct implementation of SpanByte buffer handling!The implementation correctly:
- Handles NULL buffers by setting sizes to 0
- Manages buffer offsets properly
- Implements buffer pinning at the right point
288-300: Robust cleanup implementation!The cleanup section correctly:
- Checks thread state before cleanup
- Verifies pinned state before unpinning
- Handles both buffers independently
targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
733-747: Solid cleanup implementation!The cleanup section correctly:
- Checks thread state before cleanup
- Safely unpins buffers
- Follows the same robust pattern as the SPI implementation
...OS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (2)
109-120: Add error handling for GetFirstElementUInt16()While the buffer pinning is correctly implemented, consider adding error handling for GetFirstElementUInt16() to ensure robustness:
writeBuffer = stack.Arg1().DereferenceArray(); if (writeBuffer != NULL) { // grab the pointer to the array by getting the first element of the array - writeData = (unsigned char *)writeBuffer->GetFirstElementUInt16(); + HRESULT hr = writeBuffer->GetFirstElementUInt16((CLR_UINT16*&)writeData); + NANOCLR_CHECK_HRESULT(hr); // get the size of the buffer by reading the number of elements in the HeapBlock array writeSize = writeBuffer->m_numOfElements; // pin the buffer writeBuffer->Pin(); }
222-223: Consider defining a constant for the timeout multiplierThe timeout calculation uses a magic number (2) for the multiplier. Consider defining this as a named constant to improve code maintainability:
+ // Timeout multiplier for long-running operations + static const uint32_t TIMEOUT_MULTIPLIER = 2; + // Use twice the estimated Duration as timeout - estimatedDurationMiliseconds *= 2; + estimatedDurationMiliseconds *= TIMEOUT_MULTIPLIER;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (1)
Learnt from: josesimoes
PR: nanoframework/nf-interpreter#3062
File: src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp:77-93
Timestamp: 2025-01-09T13:10:31.388Z
Learning: In nanoFramework SPI transactions, NULL buffers are valid cases when there is no data to send or receive. The code should handle NULL buffers by setting corresponding sizes to 0 instead of throwing exceptions.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ST_NUCLEO64_F091RC)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALX)
- GitHub Check: nf-interpreter (Nightly build) (Build_STM32_targets ORGPAL_PALTHREE)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (3)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice.cpp (3)
77-94: Well-structured variable declarations!The variables are properly initialized with appropriate types and clear separation between buffer pointers and data pointers.
137-188: Excellent implementation of SpanByte handling!The code demonstrates thorough handling of:
- Proper offset and length management from SpanByte
- Correct NULL buffer handling
- Buffer pinning for GC protection
286-302: Robust cleanup implementation!The cleanup section demonstrates excellent resource management:
- Proper thread state checking
- Safe buffer unpinning with pinned state verification
- Comprehensive handling of both read and write buffers
***NO_CI*** (cherry picked from commit f86e48f)
Description
Motivation and Context
CLR_RT_ProtectFromGC, which prevents the GC from incorrectly collecting the object. However, it does not prevent the object from being moved during heap relocation. Pinning the heap block (HB) is the correct approach. In a few places, this was not properly accounted for.How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor