Skip to content
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

[Energy] Add support to timed-transmission events - Part 5 of RTEA #362

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Vinggui
Copy link
Contributor

@Vinggui Vinggui commented Sep 14, 2022

Contribution description

This PR is part of the process of breaking PR #235 into small steps, a process nominated "Road to Energy Analysis" (RTEA). These small modifications will lead to the whole improvement started on March 2022. At the end of these steps, OT-NS will be capable of precisely estimating energy consumption, collision events (channel aware), and plot all the data in real-time as the simulation progress.

At the moment, OT-NS can not estimate energy, collisions, nor transmission energy. Also, even though OT is working well in simulation, many of its recovery properties are not correctly simulated as they would happen in real devices due to its incorrect timing events at the simulated radio part.

This PR adds functions to handle the radio-timed events (e.g., transmissions) by each simulated device.
It is still compatible to the current implementation of the OT.

This PR also introduces:

  • New event types to improve the way signals are exchanged between the device and the simulator.

Main modifications

  • New event type eventTypeRadioComm to distinguish the current operation of OT (using a confusing duplex comm eventTypeRadioReceived) from the new implementation;
  • New event type eventTypeRadioTxDone providing callbacks signals that consider transmission times of the simulated device;
  • New functions to handle timed events;
  • Compatibility is handled by maintaining both the version of functions that handle the current version of OT, and the next version capable of respecting the timed events it creates.

This PR keeps all previous commits to reduce future rebase efforts. However, relative to the timeline of RTEA, it addresses only the following files:

  • /ot-ns/dispatcher/dispatcher.go
  • /ot-ns/dispatcher/event.go
  • /ot-ns/dispatcher/Node.go

Requirements

None (if intended to be used with current Openthread repository). But, this is a walk to the road of PR #7500 in Openthread repository that will allow 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.

Roadmap

This is part 5 of the RTEA (Road to Energy Analysis) steps to PR #235 and PR #7500.

Part 1: PR #357;
Part 2: PR #8144;
Part 3: PR #359;
Part 4: PR #8152;
Part 5: This PR;
Part 6: PR #8165;
Part 7: PR #363;
Part 8: PR #8173;
Part 9: PR #365;
Part 10: PR #366;
Part 11: PR #367.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2022

Codecov Report

Merging #362 (70fbbb2) into main (c616148) will decrease coverage by 2.60%.
The diff coverage is 12.79%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
- Coverage   49.97%   47.37%   -2.61%     
==========================================
  Files          38       40       +2     
  Lines        4608     4906     +298     
==========================================
+ Hits         2303     2324      +21     
- Misses       2123     2394     +271     
- Partials      182      188       +6     
Impacted Files Coverage Δ
cli/CmdRunner.go 22.62% <0.00%> (-0.51%) ⬇️
cli/ast.go 66.66% <ø> (ø)
types/types.go 65.38% <ø> (ø)
web/site/bindata.go 99.00% <ø> (-0.01%) ⬇️
visualize/grpc/pb/visualize_grpc.pb.go 18.77% <3.63%> (-0.66%) ⬇️
dispatcher/dispatcher.go 50.12% <4.71%> (-6.98%) ⬇️
energy/core.go 7.31% <7.31%> (ø)
dispatcher/Node.go 24.66% <31.48%> (-5.61%) ⬇️
energy/node.go 40.00% <40.00%> (ø)
simulation/simulation.go 57.94% <50.00%> (-0.31%) ⬇️
... and 2 more

@EskoDijk
Copy link

Just noting for completeness that this PR addresses only the dispatcher.go file. (Changes are in 2nd commit, 1st commit was kept in to avoid rebasing later on)

@Vinggui @simonlingoogle @jwhui I see the motivation for this PR clearly, same as for the radiomodels PR (#316) and also needed for energy analysis. However I do worry about the proposed change here to integrate all of such functionality into the dispatcher which is already quite complex. The 'radiomodels' approach was rather a nice alternative that separates the event-dispatching more cleanly from different radiomodels (which may themselves have arbitrary complexity or be very simple, with or without CCA, with or without collissions / timing etc.). The models use the dispatcher as a timed event processing engine so it keeps that dispatcher close to its core features.

For the goal of RTEA the PR #316 can also provide the right events and hooks - there's also a new "TxDone" event type added ; and a new type "RadioTx" which has the same role as "RadioComm" here.

We have some choices to make on how to proceed with this: e.g.

  1. first merge in this PR, and later merge in Support for different, 'swappable' RF/timing models #316 (which will move the here added functionality from dispatcher to radiomodels package - possibly replacing some of it.)
  2. first merge in Support for different, 'swappable' RF/timing models #316, then adjust this PR based on it (if anything is still needed specifically for energy analysis)
  3. allow multiple dispatchers in OT-NS code, one "classic" dispatcher and one "radiomodels" dispatcher with a separate codebase. Then we avoid conflicts in how the design of the dispatcher should be. Both dispatchers would offer then energy analysis features, accurate timing, etc. Only the latter dispatcher can support switching radiomodels.
  4. create a new joint PR that supports the features added by this PR, but rewritten in a way that is compatible with the radiomodels PR - without introducing the full fledged radiomodels; more like preparing for it.
  5. let the OT-NS project fork in two and let each go their own way (not preferred...)

I would prefer one of 1, 2 or 3. In any case the merging of any PRs may take some time because OpenThread is also busy with other things and we are contributing tons of code ;-)

@Vinggui
Copy link
Contributor Author

Vinggui commented Sep 15, 2022

Hi @EskoDijk ,
I would like to thank you to keep contributing and commenting on these PRs. Definitely some good points were mentioned already.

Just noting for completeness that this PR addresses only the dispatcher.go file. (Changes are in 2nd commit, 1st commit was kept in to avoid rebasing later on)

Good point! I'll add it up in the next PRs.

@Vinggui @simonlingoogle @jwhui I see the motivation for this PR clearly, same as for the radiomodels PR (#316) and also needed for energy analysis. However I do worry about the proposed change here to integrate all of such functionality into the dispatcher which is already quite complex. The 'radiomodels' approach was rather a nice alternative that separates the event-dispatching more cleanly from different radiomodels (which may themselves have arbitrary complexity or be very simple, with or without CCA, with or without collissions / timing etc.). The models use the dispatcher as a timed event processing engine so it keeps that dispatcher close to its core features.

I believe that the decision to keep things mostly inside the dispatcher was based on the fact the dispatcher.go already provides similar functionalities, but now includes collision and correct timing. I believe that your contribution in PR #316 has much to improve on the default implementation, and even more on what this PR suggests.

I don't know yet if I agree with a medium model without more than one channel, nor a radio model without collisions. This should be up to the protocol to be able to handle. However, the idea of simplifying dispatcher.go seems very reasonable, allowing future development of new medium/radio models to be clear and more organized.

For the goal of RTEA the PR #316 can also provide the right events and hooks - there's also a new "TxDone" event type added ; and a new type "RadioTx" which has the same role as "RadioComm" here.

This is good. It means that:

  • This is likely the best approach for the problem;
  • That we can soon merge these ideas without many problems (I hope).

We have some choices to make on how to proceed with this: e.g.

I am excited to go with options 1 or 2. The only thing is that I still have a bit more commits to finish the steps which should include visual charts in the web view. If we go with option 2, I am concerned of having too many modifications to be implemented to make it keep working with the web part.
If possible, I hope to conclude this until tomorrow, and go right away to your code. I want to fully understand it to be able to suggest something, too. In this case, your code could synthesize everything, and make this a perfect joint PR for the final destination. (I expect to help, but let you push on that, since that will include your name/contribution).

Nevertheless, what I am doing now is just finishing the break into steps necessary for everyone to better debug/understand the initial suggestion at PR #235.

Thanks again.

@EskoDijk
Copy link

I believe that the decision to keep things mostly inside the dispatcher was based on the fact the dispatcher.go already provides similar functionalities, but now includes collision and correct timing.
Ok, clear - as for collision I don't think dispatcher had anything similar yet?

I don't know yet if I agree with a medium model without more than one channel, nor a radio model without collisions. This should be up to the protocol to be able to handle.

The "Ideal*" radiomodels I propose do handle all the channels - it's only that they do not simulate collissions. To me that seems a valid model: it tries to mimic what OTNS has until now. Maybe it shouldn't be the default simulation model, but that's up for a future discussion. I do think it needs to be there for comparison purposes. E.g. debug your new algorithm in ideal conditions vs realistic conditions. Or, do a simulation of a Thread system where nodes are connected by high-speed Ethernet.
By the way the "MutualInterference" radio model now handles all channels correctly, there are still some last bugs to fix and some open points (e.g. supporting energy-scan is still left as a task.) I agree that any radiomodel needs to cope with potential channel changes by the OT node in some clear way.
The protocols running on the OT node will in any case handle collissions, RSSI change, channels, etc.

I am excited to go with options 1 or 2.

Ok! Thanks also for taking all the time to respond and the splitting of the PRs.

@Vinggui
Copy link
Contributor Author

Vinggui commented Sep 15, 2022

The "Ideal*" radiomodels I propose do handle all the channels - it's only that they do not simulate collissions. To me that seems a valid model: it tries to mimic what OTNS has until now. Maybe it shouldn't be the default simulation model, but that's up for a future discussion. I do think it needs to be there for comparison purposes. E.g. debug your new algorithm in ideal conditions vs realistic conditions. Or, do a simulation of a Thread system where nodes are connected by high-speed Ethernet. By the way the "MutualInterference" radio model now handles all channels correctly, there are still some last bugs to fix and some open points (e.g. supporting energy-scan is still left as a task.) I agree that any radiomodel needs to cope with potential channel changes by the OT node in some clear way. The protocols running on the OT node will in any case handle collissions, RSSI change, channels, etc.

I see your point. For an experimental reason, or impact analysis, I agree with your points. Also, the "close-to-real" model as default seems a better option, and since this is an easy swap, I think it is a win-win situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants