Prematurely introduced feature in PATCH release of Mbed OS (5.11.2)

[screamerbg]

Description

In Mbed OS 5.11.2 (PATCH release), a feature was introduced to blockdevice, which breaks blockdevice drivers that do not live inside Mbed OS - #9135

This is diverging from the Mbed OS features release policy and also from principles of how code is rolled out in Mbed OS so far (Mbed OS 5.0 and onwards). CC @sg- @SenRamakri @mskedia

Additionally, this prematurely introduced feature breaks all Nuvoton based platforms that connect to Pelion, which use external SD card DMA driver (specific to Nuvoton) to store credentials and firmware images. For clarity, the impacted boards are:

• NUMAKER_IOT_M487
• NUMAKER_PFM_M487
• NUMAKER_PFM_NUC472

Due to the patch, any Pelion DM or storage example which makes use of the Nuvoton SD DMA driver (or any external blockdevice driver for Mbed OS), fails with the following error:

Error:  #322: object of abstract class type "NuSDBlockDevice" is not allowed:
            pure virtual function "mbed::BlockDevice::get_type"  has no overrider

Lastly, we don't expect that every single blockdevice driver will live inside Mbed OS, just as we don't anticipate that other drivers (IP connectivity, BLE, etc) will always live inside Mbed OS. Therefore, the patch should be reverted and removed from the Mbed OS codebase immediately, and reintroduced in Mbed OS 5.12.

I'll mark the issue as bug, although it is an issue with the release process of Mbed OS.

CC @0xc0170 @samchuarm @cyliangtw

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug

[samchuarm]

@ARMmbed/team-nuvoton

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-806

[0xc0170]

We will review the messaging for this change (release notes, commit messages - they should have answered questions asked) and do retrospective. I'll create a ticket for us (=maintainers).

[screamerbg]

@0xc0170 What's the outcome of this? I haven't seen any output. Shall I reopen this ticket for us to track it?

[0xc0170]

@dannybenor Please provide an update for closur on Github when closing an issue in Jira.

The reason for providing this in the patch is in the referenced pull request. The process update was done and can be found: https://os.mbed.com/docs/mbed-os/latest/contributing/workflow.html - Functional or Breaking changes require "Release notes" section with details about why are we doing it and how to migrate current code.
5.12 PRs including functionality changes should have this new section present. They will be used in 5.12 release notes.

[screamerbg]

@0xc0170 Thanks for the explanation. Unfortunately I don't think that this was handled in a way the process indicates.

Namely PR #9135 is marked as "Functionality change" and according to the workflow:

Functionality change
A functionality change can be any change in the functionality, including adding a new feature, a new method or a function. Software language does not matter.
A feature contribution contains a new API, capability or behavior. It does not break backward compatibility with existing APIs, capabilities or behaviors.

As explained above, this functionality contains a breaking change. Here is the definition for "Breaking change"

Breaking change
A breaking change is any change that results in breaking user space. It should have strong justification for us to consider it. Often, such changes can be backward compatible, for example, deprecating the old functionality and introducing the new replacement.
A contribution containing a breaking change is the most difficult PR to get merged. Any breaking changes in a codebase can have a large negative impact on any users of the codebase. Breaking changes are always limited to a major version release.

But the PR is not marked as a breaking change (see the empty checkbox).

I'm looking forward to learning more about the outcome / conclusion on how to improve the handling of these cases in the future.