search

shamblett / coap

A Coap package for dart
Other
16 stars 13 forks source link

fix: switch to unicast for blockwise transfer after multicast request #56

Closed JKRhb closed 2 years ago

JKRhb commented 2 years ago

This PR fixes #55 by adding a method to the blockwise layer that converts multicast to unicast requests during blockwise transfer. To do so, some networking methods are adjusted to be able to retrieve the source address from responses and to provide a different address than the initial one for requests.

JKRhb commented 2 years ago

Unfortunately, something is still missing in this PR. While the correct IP addresses are being used for message exchanges after the first block was received, the client is only communicating with one of the servers while ignoring other responses.

I haven't been able to find the right point where to fix this yet. Do you have an idea where this could be added? So far I got the impression that a new CoapExchange needs to be created for each responding server from the original multicast Exchange in the matcher.

shamblett commented 2 years ago

Yes, I believe you need an exchange for each endpoint, which I believe is a responding server, in your sense, it keeps track of token values etc. for that endpoint. The harder part may be cleaning these up after the transfer has completedd

JKRhb commented 2 years ago

Yes, I believe you need an exchange for each endpoint, which I believe is a responding server, in your sense, it keeps track of token values etc. for that endpoint. The harder part may be cleaning these up after the transfer has completedd

Thank you for your feedback! I will have another look into this issue over the next upcoming days. I guess the logic for cleaning up will probably have to be added in this method, right? https://github.com/shamblett/coap/blob/c8299af8fbcadd4cd26f7317704cd70db1d8f224/lib/src/net/coap_matcher.dart#L230-L280

shamblett commented 2 years ago

Yes, this is what I was thinking, looking at your proposed change however where only addresses are switched we may be OK here, I was more worried if when we switched IP addresses we would create another matcher, if we are not doing that then we only have to ensure we clean up at the end of the block transfer which we do OK anyway.

JKRhb commented 2 years ago

It took me pretty long but now I finally got a solution for both the "regular" and the blockwise multicast. I still need to do some cleanup but I'll hopefully be able to push it tomorrow/later today :)

JKRhb commented 2 years ago

@shamblett Sorry for all the rebase noise :/ This PR as well as #61 are now finally ready for review, though :) #61 now introduces a working solution for "regular" multicast while this PR extends it to also support blockwise (i. e. block2) transfers.

I tested both PRs thoroughly with two boards that feature both "simple" CoAP endpoints (returning a small payload in response to a GET request or toggeling a lamp after a POST request) as well as larger payloads that are split over multiple exchanges. Let me know if you see any points that could be improvd :)

shamblett commented 2 years ago

OK, I've merge #61 and as far as I can see all is OK, all the examples work OK so do you want this merged now or are you still working on it? I can merge it and test locally if you like.

JKRhb commented 2 years ago

I can merge it and test locally if you like.

That sounds good :) The PR should be ready, the only uncertainty I have is regarding Block1 transfer as I didn't have an opportunity to actually test it yet. Therefore, this could also be excluded from the PR for now.

JKRhb commented 2 years ago

(I performed a another rebase to to clean up the commit history a bit.)