Update Runtime.Native #3028
Update Runtime.Native #3028
Conversation
- Add implementation for RebootDevice overload with parameter for reboot option. - Update native.runtime assembly declaration. - Change declaration of ExecutionEngine::Reboot to accept reboot option instead of just hard reset parameter. - Update implementation of ExecutionEngine::Reboot to deal with reboot option.
nfbot
added
Type: enhancement
Area: Config-and-Build
Type: dependencies
josesimoes
added
Area: Common libs
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (4)
src/nanoFramework.Runtime.Native/nf_rt_native.cpp (2)
45-45: LGTM: Updated assembly version identifier.The change in the version identifier from
0x109F6F22to0x0EAE898Bis consistent with the assembly update.Please ensure that any documentation or dependent components referencing this version identifier are also updated accordingly.
47-47: LGTM: Incremented assembly version number.The version number update from
{ 100, 0, 9, 0 }to{ 100, 0, 10, 0 }correctly reflects a minor version bump.Please ensure that the changelog or release notes are updated to reflect this version increment and the new features added in this release.
src/nanoFramework.Runtime.Native/nf_rt_native.h (1)
13-19: LGTM! Consider adding a documentation comment.The new
RebootOptionenum is well-defined and aligns with the PR objectives. The use of power-of-2 values (except for 0) is a good practice, allowing for potential bitwise operations in the future.Consider adding a brief documentation comment above the enum to explain its purpose and usage. For example:
/// @brief Defines the available reboot options for the system. /// These options can be used with the NativeReboot method to specify the desired reboot behavior. typedef enum __nfpack RebootOption { // ... (enum members) } RebootOption;src/CLR/Core/Execution.cpp (1)
301-312: Consider handling multiple reboot options when multiple flags are setCurrently, the
if-else ifstructure means that if bothc_EnterNanoBooterandc_EnterProprietaryBooterflags are set inrebootOptions, only the first condition will be executed, and the second one will be ignored. If the intended behavior is to allow handling multiple reboot options simultaneously, consider using separateifstatements to check each flag independently.Apply this diff to handle multiple reboot options:
-if (CLR_DBG_Commands::Monitor_Reboot::c_EnterNanoBooter == - (rebootOptions & CLR_DBG_Commands::Monitor_Reboot::c_EnterNanoBooter)) +if (rebootOptions & CLR_DBG_Commands::Monitor_Reboot::c_EnterNanoBooter) { RequestToLaunchNanoBooter(0); } -else if ( - CLR_DBG_Commands::Monitor_Reboot::c_EnterProprietaryBooter == - (rebootOptions & CLR_DBG_Commands::Monitor_Reboot::c_EnterProprietaryBooter)) +if (rebootOptions & CLR_DBG_Commands::Monitor_Reboot::c_EnterProprietaryBooter) { RequestToLaunchProprietaryBootloader(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- src/CLR/Core/Execution.cpp (1 hunks)
- src/CLR/Include/nanoCLR_Debugging.h (1 hunks)
- src/CLR/Include/nanoCLR_Runtime.h (1 hunks)
- src/nanoFramework.Runtime.Native/nf_rt_native.cpp (2 hunks)
- src/nanoFramework.Runtime.Native/nf_rt_native.h (2 hunks)
- src/nanoFramework.Runtime.Native/nf_rt_native_nanoFramework_Runtime_Native_Power.cpp (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/CLR/Include/nanoCLR_Debugging.h
🧰 Additional context used
🔇 Additional comments (7)
src/nanoFramework.Runtime.Native/nf_rt_native_nanoFramework_Runtime_Native_Power.cpp (2)
9-10: Function signature updated to include reboot optionThe function signature has been correctly updated to include the
nanoFrameworkRuntimeNativeRebootOptionparameter. This change aligns with the PR objectives and allows for more flexible reboot behavior.
14-14: Reboot option retrieval and usage implemented correctlyThe reboot option is properly retrieved from the stack argument and used in the
Rebootcall. This implementation allows for different reboot behaviors based on the provided option, enhancing the functionality as intended.To ensure the
RebootOptionenum is correctly defined and theRebootmethod signature has been updated accordingly, run the following script:Also applies to: 16-16
src/nanoFramework.Runtime.Native/nf_rt_native.cpp (1)
22-22: LGTM: Updated NativeReboot method signature.The change to include
nanoFrameworkRuntimeNativeRebootOptionas a parameter aligns with the PR objectives and enhances the reboot functionality.Let's verify the consistency of this change across the codebase:
✅ Verification successful
Verified: NativeReboot method signature is consistently updated across the codebase.
All instances ofNativeReboothave been correctly modified to include thenanoFrameworkRuntimeNativeRebootOptionparameter, ensuring consistency and enhancing reboot functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of the NativeReboot method signature change. # Test: Search for NativeReboot method declarations/definitions rg --type cpp -A 5 $'NativeReboot'Length of output: 1710
src/nanoFramework.Runtime.Native/nf_rt_native.h (1)
Line range hint
1-85: Summary: Enhancements to reboot functionalityThe changes in this file successfully implement the new reboot options as described in the PR objectives. The addition of the
RebootOptionenum and the update to theNativeRebootmethod signature provide a more flexible and detailed control over the reboot process.These changes lay the groundwork for implementing the enhanced reboot functionality in the rest of the codebase. Ensure that these new options are properly utilized in the implementation files and that appropriate documentation is added to guide developers on using these new reboot options.
src/CLR/Include/nanoCLR_Runtime.h (1)
3691-3691: Approved: Function signature updated to support multiple reboot options.The
Rebootfunction signature has been updated to accept auint16_t rebootOptionsparameter, which aligns with the PR objective of enhancing theRebootDevicemethod to include a reboot option parameter. This change allows for more granular control over the reboot process.Please ensure that the implementation of this function correctly handles the new
rebootOptionsparameter. You may want to add a comment explaining the possible values forrebootOptionsand their meanings.src/CLR/Core/Execution.cpp (2)
314-315: Confirm thatm_iReboot_Optionsis correctly utilized elsewhereThe
rebootOptionsare stored ing_CLR_RT_ExecutionEngine.m_iReboot_Options, which implies they are used in subsequent processing. Ensure thatm_iReboot_Optionsis appropriately utilized later in the execution flow to apply the specified reboot options.Run the following script to verify the usage of
m_iReboot_Options:#!/bin/bash # Description: Verify that `m_iReboot_Options` is used after being set. # Search for references to `m_iReboot_Options` outside of its assignment. rg --type cpp 'm_iReboot_Options' -A 5 | grep -v 'g_CLR_RT_ExecutionEngine\.m_iReboot_Options = rebootOptions;'
297-297: Verify that all calls toRebootmethod are updated to the new signatureThe
Rebootmethod signature has changed from accepting aboolparameter to auint16_tparameterrebootOptions. Ensure that all calls toCLR_RT_ExecutionEngine::Rebootthroughout the codebase are updated to match the new signature to prevent potential compile-time or runtime errors.Run the following script to find any calls to
Rebootthat may still be using the oldboolparameter:
Description
Motivation and Context
RebootDeviceAPI nanoFramework.Runtime.Native#140.How Has This Been Tested?
Screenshots
Types of changes
Checklist
CC @gligorov
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Version Updates