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

Parse Wifi SSIDs and convert to Capture and add back NavVis test data #68

Merged
merged 4 commits into from
Jun 20, 2024

Conversation

vjlux
Copy link
Collaborator

@vjlux vjlux commented Jun 19, 2024

This PR does 2 things

1 Parse and convert the Wifi SSIDs from NavVis into Capture

2 Add NavVis test data

@vjlux vjlux requested review from pablospe and MarioGini and removed request for pablospe and omiksik June 19, 2024 09:33
@omiksik
Copy link

omiksik commented Jun 19, 2024

image
Thanks but we seem to commit many new files (images, ...) - is this on purpose?

@vjlux
Copy link
Collaborator Author

vjlux commented Jun 19, 2024

image Thanks but we seem to commit many new files (images, ...) - is this on purpose?

All unit tests were failing because the test data wasn't there. I have no context why the test data has been removed. I added it back in from https://github.com/microsoft/scantools/tree/main/test_data

@omiksik
Copy link

omiksik commented Jun 19, 2024

Sounds like we should split this to 2 different PRs then?

@pablospe
Copy link
Contributor

All unit tests were failing because the test data wasn't there. I have no context why the test data has been removed. I added it back in from https://github.com/microsoft/scantools/tree/main/test_data

Notice this is another repo.

@pablospe
Copy link
Contributor

As alternative, we could do something similar to the NavVisSample:
https://github.com/microsoft/lamar-benchmark/releases/download/v1.0/NavVisSample.zip

Would that work for you?

This reverts commit 3b984d4.
@vjlux
Copy link
Collaborator Author

vjlux commented Jun 19, 2024

All unit tests were failing because the test data wasn't there. I have no context why the test data has been removed. I added it back in from https://github.com/microsoft/scantools/tree/main/test_data

Notice this is another repo.

Yes, the data is from another repo but the question is:
Why do we have tests in scantools which all depend on it? Maybe there was a specific decision made.

@vjlux
Copy link
Collaborator Author

vjlux commented Jun 19, 2024

Sounds like we should split this to 2 different PRs then?

Done: #69

@vjlux
Copy link
Collaborator Author

vjlux commented Jun 19, 2024

As alternative, we could do something similar to the NavVisSample: https://github.com/microsoft/lamar-benchmark/releases/download/v1.0/NavVisSample.zip

Would that work for you?

Hm.. I would rather create a submodule with test data

@pablospe
Copy link
Contributor

Yes, the data is from another repo but the question is: Why do we have tests in scantools which all depend on it? Maybe there was a specific decision made.

do you mean the scantools in this repo? Or in the non-public one?

@@ -213,10 +213,12 @@ def run(input_path: Path, capture: Capture, tiles_format: str, session_id: Optio
freq_khz = measurement.center_channel_freq_khz
rssi_dbm = measurement.signal_strength_dbm
time_offset_us = int(measurement.time_offset_ms) / 1_000
ssid = measurement.ssid
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

locals like these do not bring much value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know.. was rather following the pattern

@@ -49,15 +52,17 @@ def parse_iwconfig(data):
'mac_address': r'Address:\s((?:[a-fA-F0-9]{2}[:|\-]?){6})',
'signal_level': r'Signal level=(-?\d{1,2}) dBm',
'frequency': r'Frequency:(\d{1,2}.\d{1,6}\s([ G|M|k]Hz))',
'time_offset': r'Extra: Last beacon: (\d{1,6})ms'
'time_offset': r'Extra: Last beacon: (\d{1,6})ms',
'ssid': r'ESSID:"(.{1,64})"'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ESSID - sorry, not an expert - this contains same value as SSID below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. so ESSID and SSID are practically the same.

@@ -76,12 +81,14 @@ def parse_iwconfig(data):
assert -127 <= float(wifi_sample['signal_level']) <= 127
assert wifi_sample['frequency'] is not None
assert wifi_sample['time_offset'] is not None
assert wifi_sample['ssid'] is not None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we verify we can do this? we've recently found that some WiFi samples on some other platforms do not contain ssid so flagging here whether we're certain this is always available

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For NavVis I have not seen any wifi sample without it. And I would argue we should also be strict here. Thanks for checking.

@vjlux vjlux force-pushed the user/lugruber/parse_ssids branch from f2ae3cb to 0015cd8 Compare June 20, 2024 13:59
@vjlux vjlux merged commit f4d83cb into main Jun 20, 2024
2 checks passed
@vjlux vjlux deleted the user/lugruber/parse_ssids branch June 20, 2024 14:18
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