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

Visualization of Frames (currently only IWLFrame) #6

Merged
merged 35 commits into from
Apr 23, 2021

Conversation

tweigel-dev
Copy link
Contributor

Hay,
as announced, here is my implementation for plotting Metrics of Intel5300.
Could you give me feedback to this implementation and give a example what you would like to plot and I will try to deliver it with this implementation.
It work as descriped at ipynb in ./docs/ folder

The main task is to plot different dat files. For each metric you choose it will plot one subplot.
If you want only one Metric to plot, fit only one measurement and one entry into the scenario or use the PlotImementation directly.

This mechanism could be easily extended to plot the data of other NICs.
Just inherit the Metric class and override the notice.

@tweigel-dev tweigel-dev changed the title [Feedback] [Feedback] Visualization of Frames (currently only IWLFrame) Feb 11, 2021
@Gi-z
Copy link
Owner

Gi-z commented Feb 11, 2021

Thanks for this, I should hopefully have time to look through this in the next few days. Looking forward to merging this.

@tweigel-dev
Copy link
Contributor Author

Currently, I am try to implement the colormap amplitude per subcarrier as plot as example. It will follow in a few days

@tweigel-dev tweigel-dev changed the title [Feedback] Visualization of Frames (currently only IWLFrame) [WIP] Visualization of Frames (currently only IWLFrame) Feb 11, 2021
@Gi-z
Copy link
Owner

Gi-z commented Feb 24, 2021

Just want to apologise for being so slow with this one. Got my IWL5300 system back up and running again so I should be good to test and get this merged soon.

@tweigel-dev
Copy link
Contributor Author

Hay,
The Changes are Not finished yet and still Work in progress.
In a few weeks i will have the time to complete this.

@tweigel-dev tweigel-dev changed the title [WIP] Visualization of Frames (currently only IWLFrame) Visualization of Frames (currently only IWLFrame) Mar 5, 2021
@tweigel-dev
Copy link
Contributor Author

tweigel-dev commented Mar 5, 2021

So now it should work.
But there is one known issue. If you want to save a Image of an Graph which use multiple axes, it will override the resulting pdf.

please try to use it and give me feedback to the usability

@tweigel-dev
Copy link
Contributor Author

@Gi-z after merging the last changes the import fails with the following error.
Did you understand what happens here?

ImportError                               Traceback (most recent call last)
<ipython-input-1-e602741f5b34> in <module>
      2 get_ipython().run_line_magic('autoreload', '2')
      3 import json
----> 4 from CSIKit.reader import IWLBeamformReader
      5 from CSIKit.visualization.graph import *
      6 from CSIKit.visualization.metric import *

~/Documents/workspace/rim/CSIKit/CSIKit/reader/__init__.py in <module>
      1 from CSIKit.reader.reader import Reader
      2 
----> 3 from CSIKit.reader.readers.read_atheros import ATHBeamformReader
      4 from CSIKit.reader.readers.read_bfee import IWLBeamformReader
      5 from CSIKit.reader.readers.read_pcap import NEXBeamformReader

~/Documents/workspace/rim/CSIKit/CSIKit/reader/readers/read_atheros.py in <module>
      4 from math import floor
      5 
----> 6 from CSIKit.csi import CSIData
      7 from CSIKit.csi.frames import ATHCSIFrame
      8 from CSIKit.reader import Reader

~/Documents/workspace/rim/CSIKit/CSIKit/csi/__init__.py in <module>
----> 1 from CSIKit.csi.csidata import CSIData
      2 from CSIKit.csi.csiframe import CSIFrame
      3 from CSIKit.csi.frames.iwl import IWLCSIFrame
      4 from CSIKit.csi.csimetadata import CSIMetadata
      5 

~/Documents/workspace/rim/CSIKit/CSIKit/csi/csidata.py in <module>
----> 1 from CSIKit.csi import CSIFrame
      2 from CSIKit.csi import CSIMetadata
      3 from CSIKit.util.csitools import get_CSI
      4 
      5 class CSIData:

ImportError: cannot import name 'CSIFrame' from partially initialized module 'CSIKit.csi' (most likely due to a circular import) (/home/thomas/Documents/workspace/rim/CSIKit/CSIKit/csi/__init__.py)

@Gi-z
Copy link
Owner

Gi-z commented Mar 5, 2021

Thanks for continuing with this.

That issue is a circular import, likely caused by me not ordering the imports correctly. They occur when you attempt to import a module which itself requires the original module, or vice versa. If you follow the imports as shown in the error, you can likely find where this is happening and reorder the imports yourself. If not, I'm more than happy to resolve this once I've got some time. Looking forward to trying this out.

@tweigel-dev
Copy link
Contributor Author

it should work now :) I found the issue at the CSIKit/csi dir.
Looking forward to your feedback!

@Gi-z
Copy link
Owner

Gi-z commented Mar 8, 2021

Just started looking at this last night. Really interesting stuff, think we might want to move forward with this style for letting people put together their own visualisations in a flexible way. Going to finish going through it and we can figure out how to move on from there 👍

@tweigel-dev
Copy link
Contributor Author

tweigel-dev commented Mar 12, 2021

to expand the visualization to the data of the other chips we could inherit the metrics class.
To customize the graphs the user should have access to the "axes" and the belonging "figure" of matplotlib.
One big issue is the access to the axes. An other one is that the different graphs have one or multiple axes.
Matplotlib is not konsistent in object-oriented xor functional operations. Therefore there are some global contexts at matplotlib which makes the thing difficult. It seems that matplotlib move primary to the object oriented type of usage. But this seems to be work in progress. One way would be to wait until the changes at matplotlib are finished, or implement a own workaround about the bindung between figure and axes. we could implement an own "container" for the axes or inherit figure.

I Think after we finished this we have expand the functionality of the visualization to real time plots.
seemoo-lab/nexmon_csi#192
https://github.com/Gi-z/CSIKit/blob/master/CSIKit/legacy/realtime_graph.py
#3
offside Topic:
What did you think about the idea of using and implementing testing at this repo?

@Gi-z
Copy link
Owner

Gi-z commented Mar 12, 2021

Completely agree and I'd be happy to look at realtime visualisation once we've merged this functionality.
I'm going to take a deep dive tomorrow and provide some feedback on how we can merge the coding styles. Nothing too excessive, just need to make sure the structure is mostly similar. I also need to get some more complete documentation done at some point, so this'll need to get documented too in the future.

@Gi-z
Copy link
Owner

Gi-z commented Mar 18, 2021

Going to look into this today and give some feedback about how we can merge coding styles. Once this is done, I'm hoping to do an optimisation pass on the parsers as I'm unhappy with their performance.

@Gi-z
Copy link
Owner

Gi-z commented Mar 19, 2021

Alright, finally had some time to look at this in detail. Great work overall, this is exactly the level of insight I was looking for.

As for merging, it's pretty much good to go. I've got a few small changes I'd like to suggest which should hopefully help merge it with the existing codebase cleanly. If you'd like to give me write access to your fork, I'd be happy to push them myself, otherwise I can post a diff or create a feature branch on this repo based on your fork.

If you could look into the following points, I'd really appreciate it as they'd help with the merging process.

  • Type Hints for method signatures (https://docs.python.org/3/library/typing.html)
  • Some functions have been recreated, which already exist within the main codebase (e.g. get_total_rssi). It'd be great if we could centralise these functions so they only exist in one place and serve both.
  • If you could break down the Metric file into separate files where possible, it'd help with readability. The code is great and I've been learning a few things (I didn't know you could use {} comprehension without lists!), it'd just be easier to understand the whole Metric system you've designed if it were just in 2-3 focussed files.

For documentation, this'll need a lot of consideration. I'm hoping to complete an optimisation pass on the parsers and surrounding systems as my next stage of this project, after which I'd really like to take a deep dive into documentation and make CSIKit more accessible for people.

Overall I'm really happy with this contribution. Thanks for working on this.

@tweigel-dev
Copy link
Contributor Author

Thanks for your feedback.
You got access to the repo. please work on an feature/
Looking forward to merge this

@Gi-z
Copy link
Owner

Gi-z commented Mar 24, 2021

Pushed my first set of changes. Mostly formatting changes just to ensure readability, but I've also reduced some import usage where possible (statistics.stdev -> np.std). Since we're committed to using numpy, may as well benefit from the functionality therein rather than getting it from elsewhere.

I hope to finish my changes soon, as we'll make this the 1.1.0 release (additional features without any breaking changes).

@Gi-z
Copy link
Owner

Gi-z commented Apr 21, 2021

Apologies for the repeated delays. I think we're about ready to merge this. Would you be happy if I reshuffled some parts of the example jupyter notebook you included, maybe leaving some spots for you to explain some of the functionality, and then we should be good to deploy?

@tweigel-dev
Copy link
Contributor Author

Sure

@Gi-z Gi-z merged commit 336a716 into Gi-z:master Apr 23, 2021
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.

2 participants