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

Wifi integration branch #309

Merged
merged 39 commits into from
Aug 2, 2024
Merged

Wifi integration branch #309

merged 39 commits into from
Aug 2, 2024

Conversation

LnnrtS
Copy link
Collaborator

@LnnrtS LnnrtS commented Jul 22, 2024

This combines #288, #283 and #300

LnnrtS and others added 26 commits June 28, 2024 13:45
- increase serial baudrate
- add separate management channel
- query ip and port over management channel
- wifi connect in separate task
- estimate file upload timeouts in frontend
- remove (arbitrary) file upload limits in frontend
- heartbeat initiated from metamodule every second
- stream file upload requests from web client to metamodule
- potential fix for micropython reformatting filesystem partition
- update to micropython v1.23.0
@LnnrtS LnnrtS force-pushed the wifi_integration_branch branch from 29b718e to 3bd90f6 Compare July 22, 2024 08:15
This was referenced Jul 22, 2024
@LnnrtS LnnrtS marked this pull request as ready for review July 22, 2024 11:36
@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 23, 2024

The first time I upload a file, it works. If I try to upload the same again immediately, it fails

Could not reproduce that here. It sends the request normally for twice the same file in a row. However the second message is dropped due to the other effects already listed above.

I made some smaller improvements for the timeout (removed the client side one and set a large one the time between request sent and response received at the serial port). This hopefully will make things clearer. It also fixes the problem where multiple requests queue up on the client side (subsequent messages are still dropped at the uart receiver but that's another problem).

Progress indication is in the making.

@LnnrtS LnnrtS force-pushed the wifi_integration_branch branch from 25e9ee5 to 821873c Compare July 23, 2024 10:21
@danngreen
Copy link
Member

The first time I upload a file, it works. If I try to upload the same again immediately, it fails

Could not reproduce that here. It sends the request normally for twice the same file in a row. However the second message is dropped due to the other effects already listed above.

Ok, this is strange. I tried macos Chrome and Safari and have the same results.
But then I tried Firefox, and it doesn't have this behavior. So it must be something related to the browser.
I tried uploading files with Chrome and Safari on other sites and I can upload the same file multiple times in a row, no problem.
It's definitely not an issue of the M4 being busy, or multiple requests getting queued because it doesn't matter how long you wait in between uploads. You can even eject the media and re-insert it. It seems that the absolute path has to be in the same in order for it to ignore the request, but the amount of time or the contents of the file don't matter.
Also interesting: it doesn't matter which drive I upload to each time, it still ignores requests.
Not sure, but could it be related to this?

 uploadFileSelection.addEventListener("change", uploadPatch);

If the abs path doesn't change, then maybe browsers interpret differently whether to send the change event?

I made some smaller improvements for the timeout (removed the client side one and set a large one the time between request sent and response received at the serial port). This hopefully will make things clearer. It also fixes the problem where multiple requests queue up on the client side (subsequent messages are still dropped at the uart receiver but that's another problem).

Ok, I haven't seen any issues but will keep testing.

Progress indication is in the making.

Cool. Yeah we can just show it in the browser for now. The A7 side responds to an upload if you have the patch selector page open, because it refreshes. That's good enough for v1.0.

LnnrtS added 5 commits July 25, 2024 15:36
- after uploading a patch, first broadcast a new patch list before closing the request
- only send updated patchlist if currently not receiving a frame
- fix uploading identical files after each other ignored on some browsers
- only allow a single in-flight synchronous request at the webserver
- add progress bar for patch file uploads
@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 25, 2024

If the abs path doesn't change, then maybe browsers interpret differently whether to send the change event?

Yes, exactly, that was it. I was just testing with firefox so I didn't notice that one.

I made changes to requests are sequenced on the webserver. Now I can upload the same 200kB file twice, both requests are in-flight from the browser side and they both land on the metamodule one after another without overruns.
Without proper multitasking we cannot ensure we don't run into overruns though. For example while the GUI saves a large patch file (or even rebuilds the patch list) the wifi will also not be handled for a while and messages get dropped.

There is also progress indication now. It's a bit of hack still but should show the principle. I need to refactor this later. In the meantime, let me know if you hit any corner cases.

Regarding progress indication on the A7: Until a message has been fully received, the M4 can't tell what's inside so it can't even tell if it's a patch upload. But just a notification on complete upload is pretty easy to do (for post v1.0)

@danngreen
Copy link
Member

OK, looking good! I tested a few cases, and so far no issues.
The progress bar, while not really accurate, it still gives a psychological response that "somethings happening", which is what the UX needed.

Without proper multitasking we cannot ensure we don't run into overruns though. For example while the GUI saves a large patch file (or even rebuilds the patch list) the wifi will also not be handled for a while and messages get dropped.

Right. I got some "soft overrun" errors when sending multiple files. We might be able to address this easily by putting the wifi parser in a higher priority ISR.
We also might be able to simplify the directory tree rebuilding. We don't actually need to scan the entire drive since we know which node changed, we could just directly modify that. Somehow. Need to look at it some more...

@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 26, 2024

The progress bar, while not really accurate, it still gives a psychological response that "somethings happening", which is what the UX needed.

Yeah, there is only limited information available in browser. For one it only gives process for the upload part, not the processing of the received message and writing to disk in the metamodule. Then the upload is only from the browser into the TCP stack of the receiver. If I run the webserver on my laptop, the upload is immediate because the TCP receive window is 1MB. So the progress bar goes to 100% and stays there for a long time. Interestingly, one can somewhat simulate the hardware behaviour by enabling throttling in the browser's developers tools.

@danngreen
Copy link
Member

OK, trying to merge this, but CI is failing for std::expected:
https://github.com/4ms/metamodule/actions/runs/10100870745/job/27933194779

I thought we checked for this already. But clang++ 18 fails to build even a simple test program that has std::expected on my local linux machine (clang 18.1.3). Not sure what's going on. I also tried install the extra libc++ packages as described here, but that doesn't work:
llvm/llvm-project#62801

Adding the flag -D__cpp_concepts=202002L does work, but we get a warning (and who knows what else that flag might change?)

@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 26, 2024

OK, interesting. Seems to be the combinating clang with libstdcxx and somewhat host specific. Since its just the simulator I would go with whatever fix seems least dirty and deal with it later.

@danngreen
Copy link
Member

Ok, I think I figured it out. The libc++-18-dev and libc++api-18-dev packages need to be installed AND clang++ needs the -stdlib=libc++ flag.

@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 26, 2024

OK, nice! That probably also needs to go to the docs, right?

@danngreen
Copy link
Member

Well... fixed on issue and another popped up:

undefined reference to `std::__1::__fs::filesystem::__absolute(std::__1::__fs::filesystem::path const&, std::__1::error_code*)'

And a lot of other errors about missing symbols:
https://github.com/4ms/metamodule/actions/runs/10116306299/job/27978881835

@danngreen
Copy link
Member

Is it easier just to roll our own std::expected replacement? We're not doing anything complicated with it.

@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 26, 2024

Weird. Probably from replacing the stdlib..

We could use one of those polyfills mentioned in the llvm issue tracker (and switch back later when that issue hopefully just goes away with a version upgrade). The polyfill could also be just for clang 18 or in case the symbol was not defined after including the correct header. I don't want to overengineer, just not persisting a workaround for whay is actually just a quirk of one platform

@danngreen
Copy link
Member

Yeah, the polyfill is probably best. If we set it up right, we can change it easily when the time comes

@LnnrtS
Copy link
Collaborator Author

LnnrtS commented Jul 29, 2024

I tried to use the polyfill but the impact on the cmake code was rather large.
Looking again into clang issue it looks like they realized their feature test macro can safely be upgraded, so just overwriting that in the simulator is probably safe. Plus this fix is just a few lines and not touching the firmware build.

We should probably clean up the history before merging (consolidate wifi image updates and simulator fixes with reverts)

@danngreen
Copy link
Member

OK, sorry for the delay. I'm sqaushing and merging this now.

@danngreen danngreen merged commit ad42e9c into main Aug 2, 2024
4 checks passed
@danngreen danngreen deleted the wifi_integration_branch branch August 7, 2024 17:15
@danngreen danngreen restored the wifi_integration_branch branch August 7, 2024 17:24
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.

2 participants