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

Handle Bluetooth connection dropping #71

Closed
matthudsonau opened this issue Mar 6, 2024 · 20 comments · Fixed by #74
Closed

Handle Bluetooth connection dropping #71

matthudsonau opened this issue Mar 6, 2024 · 20 comments · Fixed by #74
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@matthudsonau
Copy link
Contributor

At the moment there is no monitoring of the connection status by the remote, so often the only indication that the connection has failed is that the camera stops responding

@matthudsonau
Copy link
Contributor Author

I've got a mostly ok fix on a branch - it doesn't do any checking while in a menu/submenu, but once you're in a function it'll kick you out if the connection drops and send you back to the master menu

It'd be nice to get it working in the submenu, and even attempting a reconnect on drop

(side note: I'm back. Had a rental inspection to deal with and work kicked up a bit at the same time. All done for now thankfully)

@matthudsonau
Copy link
Contributor Author

Ok, so it's not too hard to detect when the connection drops with an event, then have that event function handle the reconnect. The only headache is that if the connection drops while you're in a menu, then it doesn't return correctly once the reconnect is done (something to do with the progress bar). Annoyingly M5ez hasn't been updated in ~4 years, so I'm guessing that's a dead project

@gkoh
Copy link
Owner

gkoh commented Mar 6, 2024

Welcome back.

I wondered how long before someone got annoyed enough to fix this.
I did take a stab at it a year or so ago, but wasn't motivated enough.

At the time I implemented the onDisconnect() callback for the underlying NimBLE-Arduino library to have the connection state updated in an async manner.

@gkoh
Copy link
Owner

gkoh commented Mar 6, 2024

Ok, so it's not too hard to detect when the connection drops with an event, then have that event function handle the reconnect. The only headache is that if the connection drops while you're in a menu, then it doesn't return correctly once the reconnect is done (something to do with the progress bar). Annoyingly M5ez hasn't been updated in ~4 years, so I'm guessing that's a dead project

Yes, M5ez seems dead. The version we use is super hacked up to work on the M5StickC, but I recently reformatted it and will continue to use it (because it's EZ).

The event system does work, I use it to update GPS data in the background.

@gkoh
Copy link
Owner

gkoh commented Mar 6, 2024

I'll be interested to see what you got, because anything above what is there (nothing) is an improvement.

@gkoh gkoh added this to the v2.0.0 milestone Mar 6, 2024
@gkoh gkoh added bug Something isn't working enhancement New feature or request labels Mar 6, 2024
@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

Ok, so it's not too hard to detect when the connection drops with an event, then have that event function handle the reconnect. The only headache is that if the connection drops while you're in a menu, then it doesn't return correctly once the reconnect is done (something to do with the progress bar). Annoyingly M5ez hasn't been updated in ~4 years, so I'm guessing that's a dead project

I recall struggling with the reconnection experience.
Thinking now, auto-reconnect is might not be the most obvious/useful thing (if you go slightly out of range it could go into a disconnect-reconnect loop).
ezmsgBox() has a blocking arg, perhaps on an async disconnection, fire up a blocking dialog and force the user decide to exit or reconnect?

Not sure, just firing random thoughts.

@matthudsonau
Copy link
Contributor Author

Current behaviour of my test is to make a single reconnection attempt, then bail out if that fails (so you're back at the main menu in theory). In practice, all the menus are broken after the reconnect attempt, but if you're in the remote shutter or interval modes it works fine(ish)

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

OK, that sounds like a good improvement.
I found a fairly serious bug from the intervalometer patch, which I fixed here:
e2640e5#diff-23d4bc9ce6f672ef3cbc0bc04dae19abe006c8d03f32a3e2f7dafe0aa9ea9c9e

You may need it if you're finding crazy crashy stuff happening.

@matthudsonau
Copy link
Contributor Author

OK, so it's definitely something to do with the progress bar - I removed that part of the code and it jumps back and forth between the reconnect and submenus just fine. It'd be really good to figure out what it's doing that's making the menu break, so I'm going to go digging and see what I can find

It does have an issue dropping back to the main menu if connection totally fails, and I don't have a solution for that. Part of my brain is starting to tell me to just build my own damn menu system, but I'm pretty sure that's a terrible idea

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

OK, so it's definitely something to do with the progress bar - I removed that part of the code and it jumps back and forth between the reconnect and submenus just fine. It'd be really good to figure out what it's doing that's making the menu break, so I'm going to go digging and see what I can find

It does have an issue dropping back to the main menu if connection totally fails, and I don't have a solution for that. Part of my brain is starting to tell me to just build my own damn menu system, but I'm pretty sure that's a terrible idea

Yes, it's a terrible idea, but not one I haven't considered many times myself.
Perhaps it is time to actually consider replacing the UI framework, especially because I'd like to better support the M5Core platforms (with 3 buttons). Right now we're hacking my hacks ...

Looking around LVGL seems to provide a lot of the basic things we require:
https://lvgl.io/

There's some mention of the UIFlow stuff M5Stack offer being based on this library.
Migrating will be a 'lot of work' but worthwhile in the long term.

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

I never looked at M5Stack's own UIFlow until now, maybe something to consider:
https://flow.m5stack.com/

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

I never looked at M5Stack's own UIFlow until now, maybe something to consider: https://flow.m5stack.com/

Erg, OK, not UIFlow, it's a full-blown IDE in a browser thing.

@matthudsonau
Copy link
Contributor Author

Yeah, I had a bit of a look at UIFlow, but since it's Python instead of C++ I figured I'd skip it. My C might be rusty, but it's a damn sight better than my non-existent python skills

I'll see how much damage I can do between whatever I can remember from my uni days combined with everything I've learnt so far. Probably going to be a beautiful mess, I'll let you know how I get on

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

I suggest trying to get the reconnect working with M5ez if at all possible, if you raise a PR I can help look into any weird stuff.

Separately, we should raise an issue to investigate UI framework alternatives.
We could either:

  • find a new one
  • fix our copy of M5ez
  • something else

I'm aiming to get furble v2.0 released "soon-ish" hopefully with:

  • useful intervalometer
  • improvements for disconnect/reconnect (this issue)

I'd defer the UI revamp to after v2, eg. v3.

@matthudsonau
Copy link
Contributor Author

matthudsonau commented Mar 7, 2024

OK, so it looks like it's something to do with clearing the canvas/screen. Working on bypassing this step without breaking things

EDIT: Commenting out the following 4 lines keeps the menu working fine. Looks like rubbish, but that's probably fixable

ez.screen.clear();
if (header != "")
  ez.header.show(header);
ez.buttons.show(buttons);

@matthudsonau
Copy link
Contributor Author

Well, that was an afternoon

Turns out the major issue was the clear command sent to the buttons when the progress bar was created. Once that was discovered, actually relatively easy to get the behaviour back as expected

Changes are on my fork, have a look and let me know what I've stuffed up. Tested it on my camera and it works fairly well

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

Well, that was an afternoon

Turns out the major issue was the clear command sent to the buttons when the progress bar was created. Once that was discovered, actually relatively easy to get the behaviour back as expected

Changes are on my fork, have a look and let me know what I've stuffed up. Tested it on my camera and it works fairly well

Cool, will do tonight or tomorrow.

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

@matthudsonau Can you raise a PR for your branch?
Overall I like what you've done, but I don't like M5ez forcing us to use a global variable for the device state.

If you raise a PR and allow reviewers push/write access I can modify M5ez itself to let us do it cleaner.
This would also allow me to fix other (unrelated) parts of the code that have annoyed me.

@matthudsonau
Copy link
Contributor Author

If you raise a PR and allow reviewers push/write access I can modify M5ez itself to let us do it cleaner. This would also allow me to fix other (unrelated) parts of the code that have annoyed me.

I hope I've done that right. Between the M5 and Github, it's been a hell of a learning curve

@gkoh
Copy link
Owner

gkoh commented Mar 7, 2024

I hope I've done that right. Between the M5 and Github, it's been a hell of a learning curve

Looks good, I can push to your branch directly now.
You're doing well with github and M5, I appreciate the effort.

@gkoh gkoh linked a pull request Mar 8, 2024 that will close this issue
@gkoh gkoh closed this as completed in #74 Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants