-
Notifications
You must be signed in to change notification settings - Fork 23
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
[Simulation] Add energy analysis with exporting commands and chart visualization #235
base: main
Are you sure you want to change the base?
Conversation
This pull request introduces 3 alerts and fixes 1 when merging 1e0f326 into 941d6e2 - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 1 when merging 50adc82 into 941d6e2 - view on LGTM.com new alerts:
fixed alerts:
|
This comment was marked as resolved.
This comment was marked as resolved.
This pull request introduces 1 alert and fixes 1 when merging 7258257 into 941d6e2 - view on LGTM.com new alerts:
fixed alerts:
|
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.
Did a very brief review of the code. The overall structure seems good to me.
One major concern might be why do we need an extra streaming RPC for energy report?
I wonder if Visualize
RPC should also send energy-related events.
dispatcher/dispatcher.go
Outdated
@@ -230,7 +232,7 @@ loop: | |||
select { | |||
case f := <-d.taskChan: | |||
f() | |||
break | |||
// break //ineffective break statement. Did you mean to break out of the outer loop? |
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.
Yes, it's a redundant break
.
I think it's added to be more readable for C/C++ readers.
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.
I agree, but I was fixing some warnings with it. I don't mind leaving, but for that C/C++ readers, a simple comment like that also solves the issue, without adding warnings.
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.
will it cause warning ?
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.
I checked again, and those warnings were actually pointed by VS Code. I believe that VS Code got it right anyway, a commented break is as useful and visually representative as an unnecessary break.
@@ -217,6 +231,7 @@ service VisualizeGrpcService { | |||
// rpc Echo (EchoRequest) returns (EchoResponse); | |||
rpc Visualize (VisualizeRequest) returns (stream VisualizeEvent); | |||
rpc Command (CommandRequest) returns (CommandResponse); | |||
rpc EnergyReport (VisualizeRequest) returns (stream NetworkEnergyEvent); |
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.
Maybe we don't need an extra GRPC streaming interface? Visualize
should just work?
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.
One major concern might be why do we need an extra streaming RPC for energy report? I wonder if
Visualize
RPC should also send energy-related events.
I decided to create a new RPC stream to detach the streamed objects from what is used by the visualize.js. It looks to me that splitting both streams is the correct approach to what this "API" is serving each clients' necessity.
Not only that, but visualizer.js
filters each streamed object for every callback it receives. Using the same streaming interface would increase unnecessary filtering in both visualizer.js
and energyViewer.js
.
I don't see a reason to merge them together, but please let me know if I did not evaluate it correctly.
This pull request introduces 1 alert and fixes 1 when merging 3e23d29 into 8da999e - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 1 when merging ad023ac into 8da999e - view on LGTM.com new alerts:
fixed alerts:
|
This pull request introduces 1 alert and fixes 1 when merging c642beb into 8da999e - view on LGTM.com new alerts:
fixed alerts:
|
I realized one big problem in this simulated energy measurements: the transmission times are not simulated. So, we don't get the right timing spent on transmission state. Update: |
Codecov Report
@@ Coverage Diff @@
## main #235 +/- ##
==========================================
- Coverage 49.93% 46.12% -3.81%
==========================================
Files 38 40 +2
Lines 4604 5058 +454
==========================================
+ Hits 2299 2333 +34
- Misses 2123 2535 +412
- Partials 182 190 +8
|
This pull request introduces 2 alerts when merging eb6d545 into 8b7655f - view on LGTM.com new alerts:
|
This pull request fixes 1 alert when merging 0bc5318 into 8b7655f - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging a7729bb into 7d96f58 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging 31e9e06 into 4616010 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging c3a61f8 into 0ab7444 - view on LGTM.com fixed alerts:
|
This pull request fixes 1 alert when merging ae7a872 into 0ab7444 - view on LGTM.com fixed alerts:
|
Contribution description
This PR adds energy analysis (including charts), transmission timings, and collision analysis support to the OTNS. It is, however, compatible with the current version of the Openthread/Openthread simulated device.
This PR also introduces:
energy save [output name]
, allowing the user to store the energy results obtained out of their simulation;3 charts: Energy consumption history, energy consumption by radio state and node ID, and power consumption.
This energy simulation is based on the STM32WB55rg at 3.3V:
Main modifications
simplelogger.Debugf
"status push", showing the simulated time;eventTypeRadioComm = 6
that will substituteeventTypeRadioReceived = 1
. At the moment, both types are used to keep compatibility between the new changes of PR #7500 and the current OpenThread version, respectively.Needs check/review
I believe that the modifications should not impact any other OTNS' functionalities, but I could not yet test some of them, for example:
If you please have easy access to it, send me feedback.
Requirements
None (if intended to be used with current Openthread repository). But, there is another PR #7500 in Openthread repository that allows all the new features in the simulation to work.
Testing procedure
Just install OTNS the same way as before, using the "OTNS=1" for the building parameter.
To obtain the energy results, just use the command
energy save
, but if you want a specific name for the files, useenergy save "name"