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

Add Software Marker Widget #1174

Merged
merged 19 commits into from
Aug 28, 2023
Merged

Add Software Marker Widget #1174

merged 19 commits into from
Aug 28, 2023

Conversation

retiutut
Copy link
Member

@retiutut retiutut commented Aug 2, 2023

Closes #1091
Closes #1114

Account for the following:

  • Insert markers using clickable buttons
  • Have multiple markers (e.g. 1.0, 2.0, etc)
  • Insert markers using keyboard shortcuts (even when widget is closed)
  • Print marker and timestamp to console log
  • Time series graph to show markers over time (live and playback)
  • Networking Widget output
  • UDP marker receiver and UI in the widget Allow markers to be set via network connection #1114

Other consequential changes while reviewing keyboard shortcuts:

  • remove s shortcut to Stop Streaming
  • remove b shortcut to Start Streaming
  • remove d shortcut as it was unused (it used to be for Cyton to reset board to defaults, this would not sync with ADS1299 settings UI)
    All of the above shortcuts are no longer useful or could cause issues if accidently pressed. We can still start/stop streaming using spacebar.

@retiutut retiutut changed the base branch from master to development August 2, 2023 21:47
@retiutut
Copy link
Member Author

retiutut commented Aug 3, 2023

image

Markers making their way into OpenBCI CSV files. Loaded into spreadsheet and sorted that column. You can see the markers next to the timestamp channel.

@retiutut
Copy link
Member Author

retiutut commented Aug 3, 2023

image

We will print an approximate time that a marker was inserted into the data. Here we see that the timestamp was correct down to the millisecond!

During this test, only one marker was inserted, so this screenshot/data is not an accident.

@retiutut retiutut force-pushed the software-marker-widget branch from 160e4c5 to 13de3f7 Compare August 3, 2023 21:20
@retiutut
Copy link
Member Author

retiutut commented Aug 4, 2023

image

Markers showing up in playback!

@retiutut
Copy link
Member Author

retiutut commented Aug 4, 2023

LSL Networking Test
image

UDP Networking Test (everything is right but the sampling rate calculation is incorrect)
image

OSC Networking Test
image

Serial Networking Test
image

@retiutut retiutut force-pushed the software-marker-widget branch from c59252e to bc8e2c9 Compare August 4, 2023 22:50
@retiutut retiutut marked this pull request as ready for review August 4, 2023 22:50
Copy link
Member

@philippitts philippitts left a comment

Choose a reason for hiding this comment

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

  • Add some padding on the right side of the buttons
  • Consider making the buttons larger and splitting them into two rows
  • Autoscale the height of the graph

@retiutut
Copy link
Member Author

  • Add some padding on the right side of the buttons
  • Consider making the buttons larger and splitting them into two rows
  • Autoscale the height of the graph

I don't think autoscaling is the way to go here (for now), until users are allowed to stream in floats from another program. Then, it should be the only option for that source.

@retiutut
Copy link
Member Author

image
image

Successfully inserting markers (as floats) from external process over UDP.

@retiutut retiutut requested a review from philippitts August 15, 2023 18:58
@retiutut
Copy link
Member Author

image

Copy link
Member

@philippitts philippitts left a comment

Choose a reason for hiding this comment

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

  • It looks like there a few files where the whole file ended up as a diff for some reason. Can you double check your line endings and try restaging the files?
  • I'm not sure how to provide external triggers via something like netcat. Can you provide some information (PR comment is fine) on this? Should the external process listen or host the connection? Does the stream have to be started first?

Networking-Test-Kit/UDP/udp_receive_marker.py Outdated Show resolved Hide resolved
Networking-Test-Kit/UDP/udp_send_marker.py Outdated Show resolved Hide resolved
OpenBCI_GUI/Board.pde Outdated Show resolved Hide resolved
OpenBCI_GUI/BoardBrainflow.pde Outdated Show resolved Hide resolved
OpenBCI_GUI/BoardNull.pde Outdated Show resolved Hide resolved
OpenBCI_GUI/Grid.pde Outdated Show resolved Hide resolved
OpenBCI_GUI/Interactivity.pde Outdated Show resolved Hide resolved
OpenBCI_GUI/W_CytonImpedance.pde Outdated Show resolved Hide resolved
@philippitts
Copy link
Member

  • Add some padding on the right side of the buttons
  • Consider making the buttons larger and splitting them into two rows
  • Autoscale the height of the graph

I don't think autoscaling is the way to go here (for now), until users are allowed to stream in floats from another program. Then, it should be the only option for that source.

Maybe we should add it now that it's possible to stream in floats from an external source?

@retiutut
Copy link
Member Author

retiutut commented Aug 16, 2023

  • Add some padding on the right side of the buttons
  • Consider making the buttons larger and splitting them into two rows
  • Autoscale the height of the graph

I don't think autoscaling is the way to go here (for now), until users are allowed to stream in floats from another program. Then, it should be the only option for that source.

Maybe we should add it now that it's possible to stream in floats from an external source?

Instead of this, users now have full control over the Y Axis, up to 20.0. Autoscale is kind of weird here since all values should be greater than 0, and it may cause "bouncing" when the graph resizes. Overall, the UX is likely better without the Y axis dynamically changing.

EDIT: On second thought, users should have the freedom to send values > 20 and still see it, though it might look weird when the graph Y axis jumps from lets say 1000 to 1 after the data point leaves the graph.

I'll add Auto as an option to Vert Scale, since we have a precedent in other widgets.

  • Add autoscale option to vert scale dropdown

@retiutut
Copy link
Member Author

retiutut commented Aug 21, 2023

  • It looks like there a few files where the whole file ended up as a diff for some reason. Can you double check your line endings and try restaging the files?
  • I'm not sure how to provide external triggers via something like netcat. Can you provide some information (PR comment is fine) on this? Should the external process listen or host the connection? Does the stream have to be started first?

In the included Python example in NetworkingTestScripts folder, we send a single float over UDP to the specified IP and Port. Though, the float is packed as a byte array.

The GUI will always listen on the specified port, and this does not use multicast. When the IP and Port are changed, the GUI will remake the UDP listener after leaving the textfield or pressing enter. A message will show at the bottom of the GUI and console log. This means the GUI and external process are completely independent and either can be started at any time.

@retiutut retiutut merged commit 8eabc56 into development Aug 28, 2023
4 checks passed
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.

Add Software Marker Widget
2 participants