Planning of next releases

[JKRhb]

Considering the number of open PRs, I wanted to open this issue to discuss which PRs could go into which new release version. The open PRs in question are the following:

• Rework option system #122
• feat: add constructors and client methods for new CoAP methods #123
• chore: also run CI on macOS and Windows #124
• refactor: use HashMap for "parsing" numeric Content-Formats #125
• refactor: clean up message decoding and encoding #127
• feat!: make client config optional #128
• feat!: introcuce _sendWithStreamResponse and sendMulticast methods, refactor send and observe implementations #129
• refactor: simplify interfaces and stack implementation #130
• feat: add additional Content-Formats #133
• fix: fix parsing of empty tokens #135
• refactor!: rework CoapDefinedAddress as enum #136

I think #124 and #125 are trivial enough to actually become part of a 5.0.2 release, as they do not add a new feature or a breaking change. These could also be merged right away, I think.

#123 could become part of the next minor release, as it introduces new features without breaking changes.

The other PRs could become part of the next major release 6.0.0, as they contain breaking changes and also apply quite significant changes to the project.

Do you think this roadmap makes sense, or do you think a different approach would be more reasonable? CC @JosefWN

[shamblett]

Sounds like a plan to me, PR's 124 and 125 merged as suggested and package re published at version 5.0.2.

[JosefWN]

Hey, sorry, been really busy. Sounds like a good plan.

[JKRhb]

@shamblett Thank you for merging #123! As mentioned above, I think it could go into a version 5.1.0.

Regarding the PRs that are still open, I think the easiest ones to review would be #128 and #129. Maybe you could have a look on them? As all remaining PRs contain breaking changes, I think we should consider them for a new major 6.0.0 release.

[shamblett]

Package re published at version 5.1.0, I'll look at #128 and #129

[JKRhb]

Thank you for merging these two! I think when it comes to the remaining PRs, it might make sense to review them in the following order (of increasing complexity): #130, #127, and #122. I just rebased them to the latest upstream version.

[shamblett]

OK, #130 merged, #127 and #122 now have merge conflicts.

[JKRhb]

Thanks! I'll rebase the two against the new upstream

[JKRhb]

The two PRs are rebased and updated now :) I also included #133 as a potential addition for 6.0.0 in the checklist – in contrast to the other PRs, it's rather simple, though :)

[shamblett]

OK, after merging #127 I'm seeing two examples hang, the get_max_retransmit and ping, not sure about the get_max_retransmit but I'm sure ping used to work OK(I tend to run all the examples after any large merge). Not saying this is a problem yet, just an observation for now, I'll dig deeper.

[JKRhb]

Thanks for uncovering these problems, I will also investigate what is causing them

[JKRhb]

@shamblett I opened #135 as a fix for the ping problem :)

[shamblett]

Merged #135, this fixes ping as you say, thanks. get_max_retransmit is still hanging, I know it says it could take a while but I've run this for 45 mins or so with no response. Wondering if there is a problem with request timeout somewhere, I'll dig deeper this week if I get chance.

[shamblett]

OK, think I know whats happening with the get_max_retransmit example hanging.

It revolves round the _sendWithStreamResponse method in CoapClient. This method waits on a CoapRespondEvent, in the case when we don't get a response this of course won't be fired so although the request is correctly cancelled the request in the client will still hang.

A quick fix is to fire a psuedo-response when we have determined the request has timed out, i.e. add the following code underneath the call to cancel() in the _timerElapsed method of ReliabilityLayer -

// SJH pseudo response with same request token
      final response = CoapResponse(CoapCode.notFound, _message.type)
        ..token = _message.token;
      _exchange.fireRespond(response);

and update the the _sendWithStreamResponse to also check for timeouts -

 .takeWhile((final _) => request.isActive || request.isTimedOut );

the request now correcly completes on timeourt with a 'CoAP encountered an exception: CoapRequestTimeoutException: Request timed out after 2 retransmits.' message.

I'm not saying this is the fix for this, I'm sure there are better and more elegant solutions, it just highlights the problem.

Maybe we should have a higher level event, maybe the CoapCompleted event that indicates that a request has completed, either by getting a response, timing out or being cancelled.

[JKRhb]

Thank you for investigating this error, @shamblett! I think I found a more or less elegant solution which I will open a PR for soon :)

[shamblett]

#137 works fine and is an elegant implementation of what I was thinking of, thanks.

OK, all the examples are now running so I'm happy to proceed with the merging of the other PR's, this is an opportune time to assess where we are and remind me again of what order we want to look at these.

[JKRhb]

#137 works fine and is an elegant implementation of what I was thinking of, thanks.

That's great :)

OK, all the examples are now running so I'm happy to proceed with the merging of the other PR's, this is an opportune time to assess where we are and remind me again of what order we want to look at these.

I think we could proceed with #122 now if it looks okay to you. #136 might need some discussion, as I am not sure if reworking the defined addresses as an enum is that much of an improvement, but it would also be ready to merge I think.

[shamblett]

#122 merged, all looking OK.

[JKRhb]

Awesome, thank you! Maybe you could have a quick look at #137? As mentioned above, I am not entirely sure if it is an improvement, so we could also skip it for 6.0.0.

[shamblett]

On #136, it does add the ability to get the InternetAddress and the InternetAddressType directly from the multicast address itself whereas the original implementation didn't, this can be expanded upon if needed, also it doesn't stop the user defining any multicast address they wish as a host name, the more I look at it the more I like it. I'm happy to merge this unless anyone can think of any drawbacks.

[JKRhb]

On #136, it does add the ability to get the InternetAddress and the InternetAddressType directly from the multicast address itself whereas the original implementation didn't, this can be expanded upon if needed, also it doesn't stop the user defining any multicast address they wish as a host name, the more I look at it the more I like it. I'm happy to merge this unless anyone can think of any drawbacks.

Okay, great! :) Feel free to merge :)

[shamblett]

OK, #136 merged, are we ready for a 6.0.0? There's quite a lot gone in lately, this is probably a good time.

[JKRhb]

Thanks! Yeah, I think we are ready for 6.0.0 now :) Feel free to close this issue afterwards.

[shamblett]

HI guys, #138 has just been raised, to do with request timeouts again but a slightly different use case if you could take a look. This won't affect the 6.0.0 release, we'll fix it as a subsequent revision, unless of course a quick solution can be found.

[shamblett]

Version 6.0.0 of the package now published, a lot of work went into this, a good effort. I've raised #140 to address the list of To-do's that are building up, just a marker, these can be addressed as we go.