Skip to content

Conversation

@kb2ma
Copy link
Member

@kb2ma kb2ma commented Nov 28, 2019

Contribution description

#11445 deprecated gcoap_req_send2(), with removal scheduled for after the 2020.01 release. However, the recent merge of #9857 changed the signature of gcoap_req_send2() (and gcoap_req_send()) to include a context pointer. This PR removes gcoap_req_send2() now because any existing uses must be touched anyway.

Testing procedure

Expect a text search of the code base for the function will not find any instances.

Issues/PRs references

See #11445 for deprecation discussion.

@kb2ma kb2ma added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CoAP Area: Constrained Application Protocol implementations labels Nov 28, 2019
@kb2ma kb2ma added this to the Release 2020.01 milestone Nov 28, 2019
@kb2ma kb2ma requested a review from miri64 November 28, 2019 16:47
@smlng
Copy link
Member

smlng commented Nov 29, 2019

I'm unsure about this, especially as planned deprecation is not that far away. Hence I would suggest to simply wait until after 2020.01 and do the removal then.

@kb2ma
Copy link
Member Author

kb2ma commented Nov 30, 2019

This PR saves a step. For example, the coap-chat RIOT app includes a call to gcoap_req_send2().

Without #12837:

  • current: gcoap_req_send2(buf, len, &remote, NULL);
  • after 2020.01: gcoap_req_send2(buf, len, &remote, NULL, NULL);
  • after 2020.04: gcoap_req_send(buf, len, &remote, NULL, NULL);

With #12837:

  • current: gcoap_req_send2(buf, len, &remote, NULL);
  • after 2020.01: gcoap_req_send(buf, len, &remote, NULL, NULL);

Also #12780 suggests changing the return type for gcoap_req_send() to a signed value. I think it's a good idea, and I'd like to include that as well in 2020.01 to concentrate the pain into a single pass through the code.

@miri64
Copy link
Member

miri64 commented Dec 3, 2019

I agree with @kb2ma, as gcoap_req_send() is changed once again (the initial change caused the creation of gcoap_req_send2() in the first place) it seems non-sensical to drag gcoap_req_send2() with us until the release.

@miri64
Copy link
Member

miri64 commented Dec 3, 2019

The application repository needs to be updated anyway, and the API change will only come into effect after the release. @kb2ma are planning to provide the required change for the application repository?

@miri64
Copy link
Member

miri64 commented Dec 3, 2019

(The update to our application repositories to 2019.10 is also missing :-/)

@miri64
Copy link
Member

miri64 commented Dec 3, 2019

(see wiki doc

@kb2ma
Copy link
Member Author

kb2ma commented Dec 4, 2019

@kb2ma are planning to provide the required change for the application repository?

Sure, I can do that. Let me make sure I understand the synchronization with the applications repository. Let's assume this PR is accepted some time this month. In that coap-chat update PR, I would need to update the RIOT submodule, right? If that PR is accepted, then the RIOT submodule would point to an unreleased version of RIOT. Is that acceptable?

@kb2ma
Copy link
Member Author

kb2ma commented Dec 4, 2019

(The update to our application repositories to 2019.10 is also missing :-/)

Thanks for pointing that out. I will plan to update/test the applications repository soon.

@miri64
Copy link
Member

miri64 commented Dec 4, 2019

Sure, I can do that. Let me make sure I understand the synchronization with the applications repository. Let's assume this PR is accepted some time this month. In that coap-chat update PR, I would need to update the RIOT submodule, right? If that PR is accepted, then the RIOT submodule would point to an unreleased version of RIOT. Is that acceptable?

I would first change over to the non-2 version of that function and then for the release (when the application directory is synchronized) switch over to the 2 version. Alternatively, we skip the step with the non-2 version as you proposed. This is possible as the applications directory is bound to a specific release, not volatile master. ;-)

@fjmolinas
Copy link
Contributor

ping @miri64 @kb2ma whats the status with this one?

@miri64
Copy link
Member

miri64 commented Jan 13, 2020

RIOT-OS/applications#68 was merged, I think we can go ahead here, right @kb2ma?

@kb2ma
Copy link
Member Author

kb2ma commented Jan 13, 2020

Yes, good to go. No other references to gcoap_req_send2() in master. I have a note to follow-up on the coap-chat app once 2020.01 is released and we update the applications repository.

@fjmolinas
Copy link
Contributor

@miri64 this one is only missing an ACK! :)

Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK! (see discussion above for reasoning)

@miri64 miri64 merged commit 5bb404a into RIOT-OS:master Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CoAP Area: Constrained Application Protocol implementations CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants