-
-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add network, wireless, wireless AP and certificates deployment #313
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (11)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! I really thing that reusing those classes will improve this.
/// <summary> | ||
/// Represents the configuration for network deployment. | ||
/// </summary> | ||
internal class NetworkDeploymentConfiguration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reuse the existing one in debugger lib NetworkConfigurationPropertiesBase
@ nanoFramework.Tools.DebugLibrary.Shared\DeviceConfiguration\NetworkConfigurationPropertiesBase.cs?
This prevents duplicating code and ensure any changes (despite unlikely) are propagated to nanoff.
Same goes for all the other config classes. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not reuse the existing one in debugger lib NetworkConfigurationPropertiesBase
Because it cannot be reused properly! That's the very first thing I4ve been trying and the issue are the enums that will for example not support string as an example. It would require to write a specific deserializer that will be even more complex than the few prive classes. And there are elements that do not serialize/deserialize properly (some of the IP addresses). So, in short, I ended up creating a simplified version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also because it's confusing to have the network configuration of wifi outside of the wifi configuration! It's a source of mistakes and complications. At least, with this solution, you have all together making it more sense and being much simpler.
As another example, the certs, the size is not needed and will anyway to have to be processed again and the serialization of the string do not work properly. So, I preferred to offer base64 and path as options.
Co-authored-by: José Simões <[email protected]>
Description
Add network, wireless, wireless AP and certificates deployment
Motivation and Context
How Has This Been Tested?
On a real ESP32 S3 device
Screenshots
Types of changes
Checklist: