-
Notifications
You must be signed in to change notification settings - Fork 41
[otns] Update OTNS codelab to OTNS2. #167
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
base: main
Are you sure you want to change the base?
Conversation
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.
This all looks great, thanks for doing the update! A few comments
site/en/codelabs/openthread-network-simulator/img/00_otns_web.png
Outdated
Show resolved
Hide resolved
site/en/codelabs/openthread-network-simulator/img/00_otns_web.png
Outdated
Show resolved
Hide resolved
… screenshots for better visibility on the web page.
@Vyrastas I've now made an update based on your review. Also added one more paragraph about timestamps correlation. For merging this update we would probably have to wait until the OTNS2 PR is done (openthread/ot-ns#530) ? |
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.
Thanks for making all the image updates @EskoDijk !
Just some copyedit nits, otherwise looks good to me. And yes we should wait for the other PR to be in before submitting this.
Have you had anyone else try out this updated version of the codelab? I only reviewed from an editorial standpoint, I did not verify that it works as written.
Thanks, updated with the suggestions now.
No, not yet. I could do a first pass myself to spot any issues, using this rendering. If we know any testing candidates let me know :) What I notice there is that some screenshots (ones that are wider) need to be enlarged, preferably to fill the entire width of the text-box/content-area in some cases. As a test, I added a commit using |
Everything looks fine on the openthread.io version (which I can't share externally), except for one image which I flagged (smaller width than the others). Thanks for doing the extra work to make it look nice! |
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.
Pull Request Overview
This pull request updates the OTNS codelab to support the new OTNS2 simulator with updated instructions, images, and command outputs. The changes include updating the installation, execution, and simulation control steps as well as revising the screenshots and console outputs to reflect the OTNS2 behavior.
Comments suppressed due to low confidence (1)
site/en/codelabs/openthread-network-simulator/index.lab.md:45
- [nitpick] Consider updating the subject–verb agreement to 'Scripted simulations (with Python) are also possible.'
Scripted simulations (with Python) is also possible.
|
||
## Delete Nodes | ||
Duration: 01:00 | ||
|
||
|
||
### Delete nodes through `OTNS-CLI` | ||
|
||
Delete node 8: | ||
Delete node 5: |
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.
[nitpick] It may be helpful to add a brief clarification for readers on why the CLI deletion targets node 5 and the OTNS-Web deletion later targets the Border Router node 9, to avoid potential confusion.
Delete node 5: | |
Delete node 5: Node 5 was previously moved to form its own partition, making it a good candidate for demonstrating the CLI deletion functionality. |
Copilot uses AI. Check for mistakes.
This updates the OTNS codelab to use the new OTNS2 simulator, which is currently not yet in the OpenThread project Github. The PR for OTNS2 is currently here.
It should be merged/accepted only after we have accepted OTNS2. This could be considered a draft PR, until that time. Some verification / fixes and checking required time may still be needed.