search

OpenBCI / OpenBCI_Cyton_Library

Repository for OpenBCI Cyton Arduino Libraries
MIT License
87 stars 88 forks source link

Timer on multichar messages #69

Closed gerrievanzyl closed 7 years ago

gerrievanzyl commented 7 years ago

@aj-ptw ,

As I was messing about with the 3.0.0 firmware, I noticed that there is no timeout for multi-char messages from the GUI to the board. At least this is the way it looks to me.

Without a timeout we run the risk of getting out-of-state between the GUI and the board in the event that a character is missed (or a message incorrectly sent). For example, the board expects 'n' and receives the '' but does not receive the 'n'. The way I read the code it will be "stuck" waiting for the 'n'. If you send the next command, it will believe it is the 'n' and then process the second char for that command as a new command. (Making any sense?)

Should we not enforce a 5ms timeout (or some other value) for multi-char messages? If the timeout is reached the serial is reset to expect a command.

This seems easy to do, if you agree I can give a shot at implementing something simple and sending a pull request.

andrewjaykeller commented 7 years ago

Seems totally fine, please make the ms at least 100ms to account for old v1 firmware drivers.

Could be a cool fix.

andrewjaykeller commented 7 years ago

@gerrievanzyl i'm almost done with this. will be releasing with new release candidate

gerrievanzyl commented 7 years ago

AJ,

I have been completely overwhelmed by work the last few days.

I have my changes running and tested, but it is a little complicated and you need let me know if you are interested before I do a pull request.

My changes did two things:

  1. Some changes to the firmware to manage multi char serial commands
  2. The addition of a new serial command '`x' where x is the value of a marker that should be inserted into the SD write and the serial write stream in the place of auxData[0].

The first change has been well tested and manages all the current multi char commands.

The second has also been tested in two modes, but as you know there are so many combinations of modes (BOARD_MODE, TIME_SYNC_MODE, ACCEL_MODE, and now MARKER_MODE) that I cannot confirm it works in all of them. On the contrary, it looks to me like TIME_SYNC_MODE is not working - at least not in combination with MARKER_MODE.

The two tested modes are:

  1. BOARD_MODE_DEFAULT (ACCEL_MODE_ON and MARKER_MODE_OFF and TIME_SYNC_MODE_OFF)
  2. BOARD_MODE_MARKER (ACCEL_MODE_OFF and MARKER_MODE_ON and TIME_SYNC_MODE_OFF)

To your knowledge, has the 3.0 firmware been tested with TIME_SYNC_MODE_ON?

I can issue a pull request for all the changes and you can only accept the the Multi Char changes.

How do you want to proceed?

Regards Gerrie

On Tue, Aug 22, 2017 at 9:58 AM, AJ Keller notifications@github.com wrote:

Closed #69 https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69 via PushTheWorld/OpenBCI_32bit_Library#14 https://github.com/PushTheWorld/OpenBCI_32bit_Library/pull/14.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69#event-1215973770, or mute the thread https://github.com/notifications/unsubscribe-auth/AOSh_0VMoK9JvaC16EBh95GoHpIUZRhvks5sat5ugaJpZM4O4-tK .

gerrievanzyl commented 7 years ago

AJ,

I tried my first pull request ("EVER!")

Hope it works for you.

I can already see how I could have done it better (like not to comment code out but just to delete it), etc.

Please give me feedback.

The marker code works well. I need to do a pull to include the GUI code - will do that next

Thanks for all your great work.

Any date on the wifi modules going on sale?

Regards Gerrie

On Tue, Aug 22, 2017 at 9:58 AM, AJ Keller notifications@github.com wrote:

Closed #69 https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69 via PushTheWorld/OpenBCI_32bit_Library#14 https://github.com/PushTheWorld/OpenBCI_32bit_Library/pull/14.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69#event-1215973770, or mute the thread https://github.com/notifications/unsubscribe-auth/AOSh_0VMoK9JvaC16EBh95GoHpIUZRhvks5sat5ugaJpZM4O4-tK .

gerrievanzyl commented 7 years ago

Which version of OpenBCI_GUI should push my changes to?

Gerrie

On Mon, Aug 28, 2017 at 2:53 PM, Gerrie van Zyl gerrie@van-zyl.net wrote:

AJ,

I tried my first pull request ("EVER!")

Hope it works for you.

I can already see how I could have done it better (like not to comment code out but just to delete it), etc.

Please give me feedback.

The marker code works well. I need to do a pull to include the GUI code - will do that next

Thanks for all your great work.

Any date on the wifi modules going on sale?

Regards Gerrie

On Tue, Aug 22, 2017 at 9:58 AM, AJ Keller notifications@github.com wrote:

Closed #69 https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69 via PushTheWorld/OpenBCI_32bit_Library#14 https://github.com/PushTheWorld/OpenBCI_32bit_Library/pull/14.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69#event-1215973770, or mute the thread https://github.com/notifications/unsubscribe-auth/AOSh_0VMoK9JvaC16EBh95GoHpIUZRhvks5sat5ugaJpZM4O4-tK .

andrewjaykeller commented 7 years ago

@gerrievanzyl you did a great job! I'm so pleased with the update. As for the GUI you will need to merge into development. Be sure to use the latest development branch.

I'm sorry I made so many comments about spacing, I've been working so hard for the past two years to unify the code spacing of any function that gets added or modified.

The GUI is really in development and it's getting more stable by the day. You will need to get the latest OpenBCI_Ganglion_Electron release and follow the instructions for running from processing. Feel free to open an issue in the GUI for this feature.

Wifi shields should be on sale by the end of next month.

Once again, I love this feature you added! So cool! People will really use this

gerrievanzyl commented 7 years ago

I will look into GUI - I assume your git repo - not the OpenBCI one

No problem about spacing - I am learning!

What do I do now? Do I make the changes on my side and then pull again?

Glad to help and that it may be useful.

Gerrie

On Mon, Aug 28, 2017 at 4:11 PM, AJ Keller notifications@github.com wrote:

@gerrievanzyl https://github.com/gerrievanzyl you did a great job! I'm so pleased with the update. As for the GUI you will need to merge into development. Be sure to use the latest development branch.

I'm sorry I made so many comments about spacing, I've been working so hard for the past two years to unify the code spacing and such of any function that gets added or modified.

The GUI is really in development and it's getting more stable by the day.

Wifi shields should be on sale by the end of next month.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69#issuecomment-325466244, or mute the thread https://github.com/notifications/unsubscribe-auth/AOSh_1ZRKRyhDLKmMHSMTGyYuXp2jLBKks5scx70gaJpZM4O4-tK .

andrewjaykeller commented 7 years ago

Make the changes, add commit and push again. Adding another commit to the same branch will allow add it to this pull request.

On Aug 28, 2017, at 4:19 PM, gerrievanzyl notifications@github.com wrote:

I will look into GUI - I assume your git repo - not the OpenBCI one

No problem about spacing - I am learning!

What do I do now? Do I make the changes on my side and then pull again?

Glad to help and that it may be useful.

Gerrie

On Mon, Aug 28, 2017 at 4:11 PM, AJ Keller notifications@github.com wrote:

@gerrievanzyl https://github.com/gerrievanzyl you did a great job! I'm so pleased with the update. As for the GUI you will need to merge into development. Be sure to use the latest development branch.

I'm sorry I made so many comments about spacing, I've been working so hard for the past two years to unify the code spacing and such of any function that gets added or modified.

The GUI is really in development and it's getting more stable by the day.

Wifi shields should be on sale by the end of next month.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69#issuecomment-325466244, or mute the thread https://github.com/notifications/unsubscribe-auth/AOSh_1ZRKRyhDLKmMHSMTGyYuXp2jLBKks5scx70gaJpZM4O4-tK .

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/OpenBCI/OpenBCI_32bit_Library/issues/69#issuecomment-325468839, or mute the thread https://github.com/notifications/unsubscribe-auth/AK6FncwOtB_v6aipIlyzQCXRaPQgzoWQks5scyDpgaJpZM4O4-tK.

andrewjaykeller commented 7 years ago

@gerrievanzyl yes please use the development branch on aj-ptw/OpenBCI_GUI/tree/development