Adding support for OEM SKU and Serial Numbers #3030
Conversation
- Add declarations and weak implementation for several platforms.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce two new functions, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
participant ConfigurationManager
User->>System: Request OEM Model SKU
System->>ConfigurationManager: ConfigurationManager_GetOemModelSku(model, modelSkuSize)
ConfigurationManager-->>System: Return OEM Model SKU
User->>System: Request Serial Numbers
System->>ConfigurationManager: ConfigurationManager_GetSerialNumbers(serialNumbers, serialNumbersSize)
ConfigurationManager-->>System: Return Serial Numbers
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: 6
🧹 Outside diff range and nitpick comments (5)
src/HAL/nanoHAL_SystemInformation.cpp (3)
36-36: LGTM. Consider adding error handling.The introduction of
ConfigurationManager_GetOemModelSkualigns with the PR objectives and provides a more modular approach for retrieving the OEM model SKU.Consider adding error handling to account for potential failures in retrieving the OEM model SKU:
- ConfigurationManager_GetOemModelSku((char *)&systemInfo.m_OemModelInfo, sizeof(systemInfo.m_OemModelInfo)); + if (ConfigurationManager_GetOemModelSku((char *)&systemInfo.m_OemModelInfo, sizeof(systemInfo.m_OemModelInfo)) != TRUE) + { + // Handle error or log warning + }
39-41: LGTM. Consider adding error handling.The introduction of
ConfigurationManager_GetSerialNumbersaligns with the PR objectives and provides a more modular approach for retrieving serial numbers. The multi-line function call improves readability.Consider adding error handling to account for potential failures in retrieving the serial numbers:
- ConfigurationManager_GetSerialNumbers( - (char *)&systemInfo.m_OemSerialNumbers, - sizeof(systemInfo.m_OemSerialNumbers)); + if (ConfigurationManager_GetSerialNumbers( + (char *)&systemInfo.m_OemSerialNumbers, + sizeof(systemInfo.m_OemSerialNumbers)) != TRUE) + { + // Handle error or log warning + }
36-41: Overall changes look good. Consider adding documentation.The introduction of
ConfigurationManager_GetOemModelSkuandConfigurationManager_GetSerialNumbersprovides a more flexible approach for OEM customization, aligning well with the PR objectives. The weak implementations allow for platform-specific overrides, which is beneficial for device manufacturers.Consider adding a brief comment above these new function calls to explain the rationale behind using these functions and their weak implementation nature. This would help future maintainers understand the design decision:
+ // Use weak implementations for OEM model SKU and serial numbers + // to allow platform-specific overrides if needed ConfigurationManager_GetOemModelSku((char *)&systemInfo.m_OemModelInfo, sizeof(systemInfo.m_OemModelInfo)); // OEM_SERIAL_NUMBERS: ConfigurationManager_GetSerialNumbers( (char *)&systemInfo.m_OemSerialNumbers, sizeof(systemInfo.m_OemSerialNumbers));src/HAL/Include/nanoHAL_ConfigurationManager.h (2)
221-223: LGTM! Consider adding error handling.The function declaration for
ConfigurationManager_GetOemModelSkuis well-defined and aligns with the PR objectives. The weak attribute allows for platform-specific implementations, which is a good design choice.Consider modifying the function to return a boolean indicating success or failure, which would improve error handling. For example:
bool ConfigurationManager_GetOemModelSku(char *model, size_t modelSkuSize);
221-227: Summary: New functions align well with PR objectives.The addition of
ConfigurationManager_GetOemModelSkuandConfigurationManager_GetSerialNumbersfunctions successfully implements support for OEM SKU and serial numbers as outlined in the PR objectives. The weak attribute allows for platform-specific implementations, providing the flexibility mentioned in the PR description.To further improve the implementation:
- Consider adding error handling to both functions.
- Clarify the use of plural "SerialNumbers" in the second function.
- Ensure that platform-specific implementations are provided where necessary.
- Update relevant documentation to reflect these new capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- src/HAL/Include/nanoHAL_ConfigurationManager.h (2 hunks)
- src/HAL/nanoHAL_ConfigurationManager.c (1 hunks)
- src/HAL/nanoHAL_SystemInformation.cpp (1 hunks)
- targets/AzureRTOS/Maxim/_common/targetHAL_ConfigurationManager.cpp (1 hunks)
- targets/AzureRTOS/ST/_common/targetHAL_ConfigurationManager.cpp (1 hunks)
- targets/AzureRTOS/SiliconLabs/_common/targetHAL_ConfigurationManager.cpp (1 hunks)
- targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp (1 hunks)
- targets/ESP32/_common/targetHAL_ConfigurationManager.cpp (1 hunks)
- targets/FreeRTOS/NXP/_common/targetHAL_ConfigurationManager.cpp (1 hunks)
- targets/TI_SimpleLink/_common/targetHAL_ConfigurationManager_CC13xx_26xx.cpp (1 hunks)
- targets/TI_SimpleLink/_common/targetHAL_ConfigurationManager_CC32xx.cpp (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
src/HAL/Include/nanoHAL_ConfigurationManager.h (1)
225-227: LGTM! Consider error handling and clarify "SerialNumbers".The function declaration for
ConfigurationManager_GetSerialNumbersis well-defined and aligns with the PR objectives. The weak attribute allows for platform-specific implementations, which is a good design choice.
- Consider modifying the function to return a boolean indicating success or failure, which would improve error handling. For example:
bool ConfigurationManager_GetSerialNumbers(char *serialNumbers, size_t serialNumbersSize);
- The plural form "SerialNumbers" suggests multiple serial numbers. Could you clarify if this is intentional? If it's meant to retrieve a single serial number, consider renaming it to
ConfigurationManager_GetSerialNumberfor consistency.src/HAL/nanoHAL_ConfigurationManager.c (2)
Line range hint
1-348: Overall assessment: LGTM with minor suggestionsThe addition of
ConfigurationManager_GetOemModelSkuis well-integrated into the existing codebase. It provides a default implementation while allowing for target-specific overrides, which aligns with the PR objectives. The existing functions remain unchanged, maintaining the overall structure and functionality of the configuration management system.To further improve the implementation:
- Consider adding more comprehensive error handling and input validation across all functions in this file.
- Ensure consistent use of the
__nfweakattribute for other functions that might benefit from target-specific implementations.- Update the file header comment to include information about the new OEM model SKU functionality.
343-348: Approve the new function with suggestions for improvement.The addition of
ConfigurationManager_GetOemModelSkuwith the__nfweakattribute aligns well with the PR objectives, allowing for target-specific implementations. However, consider the following improvements:
- Add input validation for
modelSkuSizeto prevent potential buffer overflows.- Include a comment explaining the expected behavior for target-specific implementations.
Consider updating the function as follows:
__nfweak void ConfigurationManager_GetOemModelSku(char *model, size_t modelSkuSize) { // Default implementation // Target-specific implementations should populate 'model' with the actual OEM model SKU if (model != NULL && modelSkuSize > 0) { memset(model, 0, modelSkuSize); } }Let's verify the function declaration in the header file:
✅ Verification successful
Function
ConfigurationManager_GetOemModelSkuDeclared Correctly in HeaderThe function
ConfigurationManager_GetOemModelSkuis properly declared insrc/HAL/Include/nanoHAL_ConfigurationManager.h, ensuring correct linkage and usage across the codebase.Suggestions for Improvement:
- Add input validation for
modelSkuSizeto prevent potential buffer overflows.- Include a comment explaining the expected behavior for target-specific implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the function is correctly declared in the header file rg -n "void\s+ConfigurationManager_GetOemModelSku\s*\(\s*char\s*\*\s*model\s*,\s*size_t\s+modelSkuSize\s*\)" src/HAL/Include/nanoHAL_ConfigurationManager.hLength of output: 241
targets/AzureRTOS/Maxim/_common/targetHAL_ConfigurationManager.cpp (1)
530-536: Summary: New function aligns with PR objectives but needs implementationThe addition of
ConfigurationManager_GetSerialNumbersaligns with the PR objective of adding support for OEM SKU and serial numbers. However, the current implementation is a placeholder and needs to be completed to actually retrieve and return the unique device ID.To ensure this change doesn't conflict with other parts of the codebase, please run the following script:
This will help verify that the weak implementation is appropriate and identify any existing usages that might need to be updated.
✅ Verification successful
Verified: Weak implementation aligns with existing patterns
The added
ConfigurationManager_GetSerialNumbersfunction follows the established pattern of weak implementations, allowing manufacturers to provide their own strong implementations. No conflicting or existing strong implementations were found, ensuring consistency and preventing conflicts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other implementations or usages of ConfigurationManager_GetSerialNumbers # Search for other implementations echo "Searching for other implementations of ConfigurationManager_GetSerialNumbers:" rg -p "ConfigurationManager_GetSerialNumbers\s*\([^)]*\)\s*{" --type cpp # Search for usages of the function echo "Searching for usages of ConfigurationManager_GetSerialNumbers:" rg -p "ConfigurationManager_GetSerialNumbers\s*\(" --type cppLength of output: 2213
Script:
#!/bin/bash # Description: Check for other implementations or usages of ConfigurationManager_GetSerialNumbers # Search for other implementations echo "Searching for other implementations of ConfigurationManager_GetSerialNumbers:" rg -e "ConfigurationManager_GetSerialNumbers\s*\([^)]*\)\s*{" --type cpp # Search for usages of the function echo "Searching for usages of ConfigurationManager_GetSerialNumbers:" rg -e "ConfigurationManager_GetSerialNumbers\s*\(" --type cppLength of output: 2165
Script:
#!/bin/bash # Description: Check for other implementations or usages of ConfigurationManager_GetSerialNumbers # Search for other implementations (strong implementations) echo "Searching for strong implementations of ConfigurationManager_GetSerialNumbers:" rg -e "ConfigurationManager_GetSerialNumbers\s*\([^)]*\)\s*\{" --type cpp # Search for usages of the function echo "Searching for usages of ConfigurationManager_GetSerialNumbers:" rg -e "ConfigurationManager_GetSerialNumbers\s*\(" --type cppLength of output: 1998
targets/ChibiOS/_common/targetHAL_ConfigurationManager.cpp (1)
645-651:⚠️ Potential issueImplement the actual serial number retrieval logic
The current implementation of
ConfigurationManager_GetSerialNumbersdoesn't actually retrieve any serial numbers. Instead, it just zeroes out the provided buffer. This doesn't align with the function's intended purpose.Consider the following improvements:
- Implement the actual logic to retrieve the device's unique ID or serial number.
- Add error handling to deal with potential failures in retrieving the serial number.
- Consider returning a boolean to indicate success or failure of the operation.
Here's a suggested improvement:
- __nfweak void ConfigurationManager_GetSerialNumbers(char *serialNumbers, size_t serialNumbersSize) + __nfweak bool ConfigurationManager_GetSerialNumbers(char *serialNumbers, size_t serialNumbersSize) { - // do the thing to get unique device ID - memset(serialNumbers, 0, serialNumbersSize); + // Implement the actual logic to get the unique device ID + // This is a placeholder and should be replaced with actual implementation + const char* deviceId = "EXAMPLE_DEVICE_ID"; + size_t idLength = strlen(deviceId); + + if (serialNumbersSize < idLength + 1) { + return false; // Buffer too small + } + + strncpy(serialNumbers, deviceId, serialNumbersSize); + serialNumbers[serialNumbersSize - 1] = '\0'; // Ensure null-termination + + return true; }To ensure this function is properly implemented across different targets, run the following script:
This will help identify any target-specific implementations that might need to be updated as well.
targets/AzureRTOS/SiliconLabs/_common/targetHAL_ConfigurationManager.cpp (1)
776-783:⚠️ Potential issueImplement proper serial number generation.
The current implementation of
ConfigurationManager_GetSerialNumbersdoesn't actually generate or retrieve a unique device ID. It simply clears the provided buffer.Consider implementing a proper method to generate or retrieve a unique device ID. For example:
__nfweak void ConfigurationManager_GetSerialNumbers(char *serialNumbers, size_t serialNumbersSize) { - // do the thing to get unique device ID - memset(serialNumbers, 0, serialNumbersSize); + // Example implementation (replace with actual unique ID generation) + uint32_t uniqueId = GetDeviceUniqueId(); // Replace with actual function to get unique ID + snprintf(serialNumbers, serialNumbersSize, "%08X", uniqueId); }To ensure this function is properly implemented across different targets, we should check for other implementations:
This will help us verify if there are target-specific implementations that might override this weak implementation.
targets/FreeRTOS/NXP/_common/targetHAL_ConfigurationManager.cpp
Outdated
Show resolved
Hide resolved
targets/TI_SimpleLink/_common/targetHAL_ConfigurationManager_CC32xx.cpp
Outdated
Show resolved
Hide resolved
targets/TI_SimpleLink/_common/targetHAL_ConfigurationManager_CC13xx_26xx.cpp
Outdated
Show resolved
Hide resolved
targets/AzureRTOS/Maxim/_common/targetHAL_ConfigurationManager.cpp
Outdated
Show resolved
Hide resolved
targets/AzureRTOS/ST/_common/targetHAL_ConfigurationManager.cpp
Outdated
Show resolved
Hide resolved
…_GetSystemSerialNumber and ConfigurationManager_GetModuleSerialNumber. Implemented and tested for ESP32_S3_ALL.
|
I'll commit the changes per platform, to make reviewing the code changes easier. The first is ESP32, as that's the platform I can test on real hardware. I've split the PS I could not build and test the ESP32_C6_THREAD target in de dev container (nanoFramework_All), there's an error triggered by IP network code. |
…769I_DISCOVERY target now has SN and compiles correctly, no SN for ST_STM32F429I_DISCOVERY and ST_NUCLEO64_F091RC targets as nanoHAL_ConfigurationManager_stubs is used instead of nanoHAL_ConfigurationManager.
|
STM32: The ST_STM32F429I_DISCOVERY and ST_NUCLEO64_F091RC targets do not include The ST_STM32F769I_DISCOVERY target does include |
|
Silabs Giant Gecko: added the device unique ID code. Build of SL_STK3701A and SL_STK3701A_REVB succeeds. Don't have the device, cannot test the code any further. |
…701A and SL_STK3701A_REVB succeeds.
Automated fixes for code style.
|
@josesimoes there are issues with the code style on the source files. Make sure to follow the project code style. Check the details here on how it works and the tools required to help you with that. |
There was a problem hiding this comment.
@frobijn just pushed a couple of minor changes.
It looks good.
OK to merge?
|
@josesimoes LGTM! |
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation