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

Fix data parser #106

Merged
merged 12 commits into from
Apr 29, 2022
Merged

Fix data parser #106

merged 12 commits into from
Apr 29, 2022

Conversation

@produceconsumerobot
Copy link
Collaborator

@nitin710 I'm unsure why the 2 nearest RTTs are being used instead of the 262351 and 2377788
image
2022-04-14_16-10-23-397219.csv

@produceconsumerobot
Copy link
Collaborator

@Nitin we need to look at parsed timestamp difference histograms that prove the timestamps are being parsed correctly.

@produceconsumerobot
Copy link
Collaborator

produceconsumerobot commented Apr 28, 2022

@Nitin we need to look at parsed data timestamp difference histograms that prove the timestamps are being parsed correctly.

@produceconsumerobot
Copy link
Collaborator

@nitin710 I thought we were adding some smarts about Q1, Q2, Q3, Q4 selection in calculateTimeSyncMap() in this PR. Is that not the case?

Copy link
Collaborator

@produceconsumerobot produceconsumerobot left a comment

Choose a reason for hiding this comment

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

Code generally looks good!
Biggest comment is that we should always parse data AND give instructive feedback to the user when timesyncs are limiting.
Additional comments:

  • We need to look at parsed data timestamp difference histograms that prove the timestamps are being parsed correctly.
  • I thought we were adding some smarts about Q1, Q2, Q3, Q4 selection in calculateTimeSyncMap() in this PR. Is that not the case?

EmotiBitDataParser/src/ofApp.cpp Outdated Show resolved Hide resolved
EmotiBitDataParser/src/ofApp.cpp Outdated Show resolved Hide resolved
EmotiBitDataParser/src/ofApp.h Show resolved Hide resolved
EmotiBitDataParser/src/ofApp.cpp Show resolved Hide resolved
@nitin710
Copy link
Collaborator Author

nitin710 commented Apr 28, 2022

Addressing Review Comments

Changelog

  • ✔️ Added parsing for 0 timestamps received
  • ✔️ Added parsing for 1 timestamp received
  • ❌ smarts about choosing timestamps from quadrants
  • ✔️ Improved messaging
  • ✔️ parsed data timestamp histograms

@nitin710
Copy link
Collaborator Author

nitin710 commented Apr 29, 2022

Timestamp Testing and analysis

File Condition TimeSync histogram PI Timesync histogram AX Timesync histogram EA Tiemsync histogram TH
Good example Always connected, healthy amount of timestamp received image image image image
bad example 1 timestamp received in the beginning image image image image
bad example 1 timestamp received in the end image image image image
bad example 0 timestamps received image image image image

Conclusions

  • The parsed timestamps diff histograms suggest that the inter-sample time in the parsed data is close to expectation.
    • Histograms created for
      • TH(sampling rate = 7.5Hz, Time period=0.13S): hist. mean timestamp ~ 0.1S, 0.2S
      • PI(sampling rate = 25Hz, Time period=0.04S) : hist. mean timestamp ~ 0.04S
      • AX(sampling rate = 25Hz, Time period=0.04S) : hist. mean timestamp ~ 0.04S
      • EA(sampling rate = 15Hz, Time period=0.06S) : hist. mean timestamp ~ 0.06S
  • The new parser was tested on the following cases
    • data with no timestamp ✔️
    • data with 1 time stamp towards the beginning ✔️
    • data with one tiemstamp towwards the end ✔️
    • data with sufficient timestamps ✔️
  • Updated the user message in case < 2 timesyncs are recorded
    • image
  • ❌ smarts about choosing timestamps from quadrants
    • This will be pushed to the next patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment