Add stubs for Sys.Devices.I2c and SPI to virtual nanoCLR #3100
Conversation
- Add stubs source files. - Add vcx projects. - Add refs to nanoCLR.
WalkthroughThis pull request introduces new modules for I2C and SPI support. It adds XML project files (filters) and Visual C++ project files (.vcxproj) for both modules, defining file organization and build configurations for Debug/Release and Win32/x64 platforms. Additionally, several native method stubs are added for I2C and SPI device classes to prepare for future implementation, each currently calling a placeholder for unimplemented functionality. The solution file and project references are updated to integrate the new modules into the existing build ecosystem. Changes
Sequence Diagram(s)sequenceDiagram
participant Managed as Managed Code
participant I2CStub as I2C Native Stub
participant Stack as CLR Stack
Managed->>I2CStub: Call NativeInit
I2CStub->>Stack: Invoke NotImplementedStub()
Stack-->>I2CStub: Return error indication
I2CStub-->>Managed: Return error
sequenceDiagram
participant Managed as Managed Code
participant SPIStub as SPI Native Stub
participant Stack as CLR Stack
Managed->>SPIStub: Call NativeOpenDevice
SPIStub->>Stack: Invoke NotImplementedStub()
Stack-->>SPIStub: Return error indication
SPIStub-->>Managed: Return error
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (5)
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 (
|
josesimoes
added
the
Platform: Virtual Device & WIN32
Automated fixes for code style.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiBusInfo_stubs.cpp (2)
9-16: Add documentation for the stub implementation.Consider adding a comment block to document:
- The purpose of this method (getting maximum SPI clock frequency)
- Expected return value range and units (Hz)
- Future implementation requirements
Would you like me to provide a documentation template for this method?
18-25: Add documentation for the stub implementation.Consider adding a comment block to document:
- The purpose of this method (getting minimum SPI clock frequency)
- Expected return value range and units (Hz)
- Future implementation requirements
Would you like me to provide a documentation template for this method?
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice_stubs.cpp (4)
8-15: Consider adding XML documentation for the disposal method.The stub implementation is appropriate for the virtual nanoCLR. Consider adding XML documentation to describe the expected behavior when this stub is replaced with actual implementation.
+/// <summary> +/// Disposes the native SPI device resources. +/// </summary> HRESULT Library_sys_dev_spi_native_System_Device_Spi_SpiDevice::DisposeNative___VOID(CLR_RT_StackFrame &stack)
17-25: Improve method declaration readability.While the stub implementation is appropriate, the method declaration split across lines could be formatted better for readability.
-HRESULT Library_sys_dev_spi_native_System_Device_Spi_SpiDevice:: - NativeTransfer___VOID__SystemSpanByte__SystemSpanByte__BOOLEAN(CLR_RT_StackFrame &stack) +HRESULT Library_sys_dev_spi_native_System_Device_Spi_SpiDevice::NativeTransfer___VOID__SystemSpanByte__SystemSpanByte__BOOLEAN( + CLR_RT_StackFrame &stack)
37-44: Add documentation for initialization parameters.Consider adding XML documentation to describe the initialization parameters and expected behavior when implemented.
+/// <summary> +/// Initializes the SPI device with configuration parameters from the managed side. +/// </summary> HRESULT Library_sys_dev_spi_native_System_Device_Spi_SpiDevice::NativeInit___VOID(CLR_RT_StackFrame &stack)
46-53: Document the return value meaning.Consider adding XML documentation to describe what the integer return value represents for device status.
+/// <summary> +/// Opens the SPI device. +/// </summary> +/// <returns> +/// Integer status code indicating the result of the operation. +/// </returns> HRESULT Library_sys_dev_spi_native_System_Device_Spi_SpiDevice::NativeOpenDevice___I4(CLR_RT_StackFrame &stack)src/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cpp (1)
9-16: Consider adding documentation for future implementation.While the stub implementation is correct, consider adding TODO comments with implementation requirements for each method to guide future development:
- NativeInit: Device initialization parameters and expected behavior
- NativeDispose: Resource cleanup requirements
- NativeTransmit: Expected data format and transfer protocol details
Example addition:
HRESULT Library_sys_dev_i2c_native_System_Device_I2c_I2cDevice::NativeInit___VOID(CLR_RT_StackFrame &stack) { + // TODO: Implement I2C device initialization + // - Initialize I2C hardware + // - Configure bus speed and addressing + // - Set up error handling NANOCLR_HEADER();Also applies to: 18-25, 27-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
targets/win32/nanoCLR/Generated/CLR_RT_InteropAssembliesTable.cppis excluded by!**/generated/**
📒 Files selected for processing (9)
src/System.Device.I2c/System.Device.I2c.filters(1 hunks)src/System.Device.I2c/System.Device.I2c.vcxproj(1 hunks)src/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cpp(1 hunks)src/System.Device.Spi/System.Device.Spi.filters(1 hunks)src/System.Device.Spi/System.Device.Spi.vcxproj(1 hunks)src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiBusInfo_stubs.cpp(1 hunks)src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice_stubs.cpp(1 hunks)targets/netcore/nanoCLR.sln(3 hunks)targets/netcore/nanoFramework.nanoCLR/nanoFramework.nanoCLR.vcxproj(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/System.Device.I2c/System.Device.I2c.filters
- src/System.Device.Spi/System.Device.Spi.filters
⏰ 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 (13)
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiBusInfo_stubs.cpp (2)
1-8: LGTM! Header and includes are properly structured.The file header contains appropriate copyright information and the necessary include for SPI functionality.
1-25: LGTM! Well-structured stub implementations.The implementation follows the correct nanoCLR patterns for stub methods and provides a good foundation for future SPI bus information functionality. The error handling is consistent, and the method signatures are appropriate for frequency values.
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice_stubs.cpp (2)
1-7: LGTM! Header and includes are properly structured.The file header contains appropriate copyright information and includes the necessary header file.
27-35: LGTM! Method structure is consistent.The stub implementation follows the established pattern and error handling approach.
src/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cpp (2)
1-8: LGTM! Proper header structure and includes.The file includes appropriate copyright notices and necessary header files.
27-29: Verify method signature matches managed code.The NativeTransmit method has a complex signature with SystemSpanByte parameters. Ensure this matches the corresponding managed code definition.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for the managed code definition of I2cDevice rg -A 5 "class.*I2cDevice"Length of output: 28
I'll run additional searches in the managed (C#) code to locate the definition of
I2cDeviceand the correspondingNativeTransmitmethod signature. This should help us verify that the native signature with the twoSystemSpanByteparameters correctly matches its managed definition.
🏁 Script executed:
#!/bin/bash # Search the repository for the managed definition of I2cDevice. rg -tcs "class\s+I2cDevice" . # Additionally, search for any managed declarations of NativeTransmit (which may appear in method signatures) rg -tcs "NativeTransmit\s*\(" .Length of output: 65
Action Required: Manual Verification of Native/Managed Signature Alignment
Our automated searches did not reveal the managed definition for
I2cDeviceor theNativeTransmitmethod (including its use ofSystemSpanByteparameters) within the repository. This indicates that the corresponding managed signature might be defined via generated metadata or in an external assembly. To ensure consistency between the native stub and the managed declaration, please manually verify that:
- The managed method (likely auto-generated or defined in metadata) for
NativeTransmitreturns aSystemDeviceI2cI2cTransferResultand accepts twoSystemSpanByteparameters.- The naming and parameter order in the native stub (
NativeTransmit___SystemDeviceI2cI2cTransferResult__SystemSpanByte__SystemSpanByte) matches the expected managed signature.Given that our automated search did not yield conclusive evidence, a manual review of the corresponding managed metadata or assembly is necessary.
src/System.Device.I2c/System.Device.I2c.vcxproj (2)
21-24: Verify stub implementation files.The project includes stub files for I2C device implementation. Ensure that these stubs are properly implemented to avoid runtime issues.
Run the following script to check the stub implementations:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check if the stub files contain proper implementation markers # Test: Search for NOT_IMPLEMENTED markers in stub files rg -A 5 "NOT_IMPLEMENTED|STUB" src/System.Device.I2c/Length of output: 55
I'll now run an additional shell script to examine the first portions and line counts of the stub files. This will help us verify the actual content and check if there are any incomplete implementation markers that may have been missed. If these files indeed have proper implementations, we should see full code instead of placeholder markers.
Please run this script and share the output for further analysis.
🏁 Script executed:
#!/bin/bash echo "=== sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cpp (first 50 lines) ===" wc -l src/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cpp head -n 50 src/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cpp echo "" echo "=== sys_dev_i2c_native.cpp (first 50 lines) ===" wc -l src/System.Device.I2c/sys_dev_i2c_native.cpp head -n 50 src/System.Device.I2c/sys_dev_i2c_native.cppLength of output: 2734
Stub Implementation Confirmed
The stub functions in both:
src/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice_stubs.cppsrc/System.Device.I2c/sys_dev_i2c_native.cppare intentionally using the
NotImplementedStub()mechanism. This indicates that the functions are purposely unimplemented as stubs, following the expected pattern without any extraneous placeholder markers.
106-106: Verify include paths are consistent.The include paths are consistent across all configurations. However, ensure that all required header files are available at these locations.
Run the following script to verify the include paths:
Also applies to: 119-119, 134-134, 151-151
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if all required header files exist in the include paths # Test: Check for essential header files in include directories for dir in "targets/win32/Include" "Include" "CLR/Include" "CLR/CorLib" "HAL/Include" "PAL/Include"; do echo "Checking $dir..." fd -e h -e hpp . "src/$dir" doneLength of output: 3535
Include Path Verification – Manual Check Recommended
The paths listed in the project file are consistent when interpreted relative to the location of the .vcxproj file (i.e. from
src/System.Device.I2c). Our script confirmed that the directories withinsrc/CLR/Include,src/CLR/CorLib,src/HAL/Include, andsrc/PAL/Includecontain the expected header files.However, the two paths:
..\..\targets\win32\Include(which resolves totargets/win32/Includeat the repository root), and..\Include(which resolves tosrc/Include),were not located when searching under a fixed
src/base. This likely happens because the script assumed all directories were undersrc/. Please manually verify that the directoriestargets/win32/Include(from the repository root) andsrc/Include(relative to the .vcxproj file) exist and that they contain the required header files.src/System.Device.Spi/System.Device.Spi.vcxproj (1)
22-24: Verify SPI stub implementations.The project includes stub files for both SPI device and bus info. Ensure proper implementation of these stubs.
Run the following script to check the stub implementations:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if the stub files contain proper implementation markers # Test: Search for NOT_IMPLEMENTED markers in stub files rg -A 5 "NOT_IMPLEMENTED|STUB" src/System.Device.Spi/Length of output: 55
Action Required: Manually Verify SPI Stub Implementations
The automated search did not reveal any "NOT_IMPLEMENTED" or "STUB" markers in the stub files. This result makes it unclear whether the stubs are intentionally fully implemented or if expected placeholders are missing. Please perform a manual review of the contents of the following files:
src/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiBusInfo_stubs.cppsrc/System.Device.Spi/sys_dev_spi_native_System_Device_Spi_SpiDevice_stubs.cppEnsure that the implementations (or explicit placeholders) in these files meet the project requirements for SPI functionality.
targets/netcore/nanoFramework.nanoCLR/nanoFramework.nanoCLR.vcxproj (1)
314-319: LGTM! Project references are properly configured.The I2C and SPI project references are correctly added alongside other device interfaces.
targets/netcore/nanoCLR.sln (3)
87-90: LGTM! Projects are properly added to the solution.The I2C and SPI projects are correctly added with proper GUIDs.
521-544: LGTM! Build configurations are properly defined.All necessary platform configurations (x86, x64, Debug, Release) are correctly defined for both projects.
582-583: LGTM! Projects are correctly organized.The I2C and SPI projects are properly grouped under the Assemblies solution folder.
***NO_CI*** (cherry picked from commit 6e5492e)
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Chores