sparkfun / OpenLog_Artemis

The OpenLog Artemis is an open source datalogger the comes preprogrammed to automatically log IMU, GPS, serial data, and various pressure, humidity, and distance sensors. All without writing a single line of code!
https://www.sparkfun.com/products/15846
Other
88 stars 47 forks source link

OLA should only go into main menu on a printable character. #125

Closed ryanneve closed 2 years ago

ryanneve commented 2 years ago

Only go into menu on character ASCII value >= 11. Prevents unwanted menu if qwiic powered BLE module generates noise on power-up.

PaulZC commented 2 years ago

Hi Ryan,

This is a good suggestion but I’m worried it would break backward-compatibility for anyone using pre-defined character sequences to do things through the menus. The first character would be lost. The peek solution if better - if it is supported in Apollo3 2.2.1? Alternatively you could pass the character to the menu function as a parameter?

Best wishes, Paul

ryanneve commented 2 years ago

I'll change it to the code below so it doesn't eat the character. if ((Serial.peek() >= 8) || ((settings.useTxRxPinsForTerminal == true) && (Serial1.peek() >= 8))) I though about using isprint() or isPrintable(), but I think those return false on CR and LF.

ryanneve commented 2 years ago

Changed to: if ((Serial.peek() >= 9) || ((settings.useTxRxPinsForTerminal == true) && (Serial1.peek() >= 9))) // is it a TAB (9) or higher

ryanneve commented 2 years ago

Paul, I'm having a strange problem with this code. It works fine on the USB port (Serial), but intermittently on the RX/TX pins (Serial1). Specifically, it seems to work the first time through loop(), but not after waking from sleep. If I change Serial1.peek() to Serial1.read() is works great. I don't see why this could be. I'll do a bit more testing to see if I can get to the bottom of this.

PaulZC commented 2 years ago

Hi Ryan,

You're not checking if there is anything in the buffer (with Serial.available() ) before you do the peek. That might be it? I don't know how peek behaves if there is nothing in the buffer.

Best wishes, Paul

ryanneve commented 2 years ago

The documentation says peek() should return -1 on an empty buffer. I think I'll put in some debug printouts to see if I can figure out what's going on.

-Ryan

On Thu, May 5, 2022 at 10:38 AM Paul @.***> wrote:

Hi Ryan,

You're not checking if there is anything in the buffer (with Serial.available() ) before you do the peek. That might be it? I don't know how peek behaves if there is nothing in the buffer.

Best wishes, Paul

— Reply to this email directly, view it on GitHub https://github.com/sparkfun/OpenLog_Artemis/pull/125#issuecomment-1118635032, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFFSEOWHUUR5BXRIZ24RF3VIPMOPANCNFSM5U5DC5OA . You are receiving this because you authored the thread.Message ID: @.***>

PaulZC commented 2 years ago

Hi Ryan,

I haven't forgotten about this PR... I'm just spending a long time sitting on the fence.

It breaks backward-compatibility for anyone who is using a non-printable character to open the main menu. Think about someone who has chosen to use a NULL (0x00) to open the menu, because they know it's not a valid menu choice.

I understand the sound reasoning behind the PR. I just worry that I'll get yelled at by someone who happens to be using a char less than 9 to open the menu. "Dude! My code worked - and now it doesn't!" and comments of that nature...

In this case, I think a configurable option would be perfect. Maybe toggled via the 'secret' debug menu?

Thanks! Paul

ryanneve commented 2 years ago

Paul, I'd be fine with an "Ignore non-printable characters to enter menu" option. Even in the debug or an experimental menu.

PaulZC commented 2 years ago

Closing... Adding this as a new debug menu option in v2.4.

PaulZC commented 2 years ago

Thanks again for the suggestion @ryanneve . Please give v2.4 a try. The code changes ended up looking like this:

https://github.com/sparkfun/OpenLog_Artemis/commit/aa931d931e815728d71afb506cca54b0a0337ab1