Cellular: Mark AT_CellularBase as deprecated #11945
Conversation
| @0xc0170 Easy to take in, only doxygen updated with deprecated info |
| Doesn't this change require a few words under the release notes section? |
| @AnttiKauppila Please do not add labels to the PRs. This has not even gone through ci so is not 'ready for merge' !! |
For a deprecation notice I would indeed say this requires something for the release notes |
| @AnttiKauppila ALL PRs require CI! We must have an all green policy. The CI itself will determine what if anything needs to run! |
| Summary is incorrect - this isn't touching |
@AnttiKauppila please address Kevin's comment |
Yes, this should be functionality change , not a patch as stated, it should include "release notes" addition |
AnttiKauppila changed the title | What functionality changes by adding a comment??? |
| CI started |
| There still seem to be a lot of in-tree users - is this going to generate half-a-dozen new build warnings? |
Deprecate a functionality should happen in a feature release not a patch. This comment adds a message to every build, a user would not expect to have it in a patch. The commit itself does not state neither why this is being deprecated, provide details there. Or is it sufficient to expect refactor - what refactor is planned?
👍 |
| CI started |
| We were instructed to mark everything in 5.15 which will be affected in Mbed OS 6. We cannot refactor the code for 5.15 so there is not much we can do now. I will update the commit message one more time. |
| If that causes more warnings in 5.15 build let's wait until we have the way of replacing the calls. It doesn't sound very useful for users if they don't have anyway of replacing it. It's good to have it as a PR though and we still can add it to the public blog post about deprecated functionality. |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
| This is a warning for deprecating a class and we will modify all existing usages to new functions. I don't care about warnings at this point, because those are informative for end developers and the only reason this PR has been made. I can of course close this one now and then in Mbed OS 6 we will just remove it without any warnings for end developers. We noticed we will have couple others which we missed for 5.15 like onboard_modem_api.h will get removed in 6. |
| In that scenario, the class would be being hard-removed in Mbed OS 6.0 anyway - there would have been no opportunity for people to switch while using the Mbed OS 5.15 release, cos there was no alternative. But this isn't really a public application API, is it? According to ARMmbed/mbed-os-5-docs#1176 it's internal for driver writers, and no deprecation is required. Perhaps adding that notice to 5.15 is sufficient. This reminds me a bit of the lwip driver -> EMAC driver switch. That was done between releases with no deprecation, because it wasn't application-facing API. We just co-ordinated with all partners for in-tree drivers. |
As it is internal functionality and there is no replacement for this one: The message as it is now here does not provide much to a user if this is internal functionality and in the current form it would mean a user would need to remove this deprecation message as they can't move to anything else (no replacement). I also would vote for closing this pull request. Blog post might be more suited in this case, to inform users what are the plans? Or might be better to have a section in 5.15 that this current functionality in this release might change in the next major release. Let's coordinate with docs team. |
|
| I'll close this, at least for 5.15. As agreed here. The documentation is being updated ARMmbed/mbed-os-5-docs#1176. |
8 participants
Description
Summary of change
Mark AT_CellularBase as to be deprecated. This class is not needed and will save about 2kB from binary size. The cellular properties this class is handling are (AT_)CellularDevice specific and will be moved there. Also getter for ATHandler and device_error will be moved most probably to (AT_)CellularDevice
Documentation
Pull request type
Test results
Reviewers
@AnttiKauppila
Release Notes
AT_CellularBase has been marked to be deprecated and the class will actually be removed in Mbed OS 6. The functionality will be moved to (AT_)CellularDevice class and all drivers will be updated accordingly if affected.
Summary of changes
AT_CellularBase has been marked to be deprecated and the class will actually be removed in Mbed OS 6. The functionality will be moved to (AT_)CellularDevice class and all drivers will be updated accordingly if affected.
Impact of changes
None
Migration actions required
None