mtd/ftl: Rename ftl_initialize_by_path to ftl_initialize

[xiaoxiang781216]

Summary

• ftl_initialize isn't used anymore after:
  • drivers/mtd/ftl/bch: Add direct writing method for FTL #16642
  • drivers/fs:Always use register_mtddriver() to register the MTD device (patch2) #16738
• required for examples: Update ftl_initialize usage in code and Kconfig after kernel side change. nuttx-apps#3147.

Impact

• API break in drivers/mtd/ftl:
  • int ftl_initialize_by_path(FAR const char *path, FAR struct mtd_dev_s *mtd, int oflags) becomes int ftl_initialize(FAR const char *path, FAR struct mtd_dev_s *mtd, int oflags).
  • Old int ftl_initialize(int minor, FAR struct mtd_dev_s *mtd) is gone. It wrapped ftl_initialize_by_path() before, now it is used directly where path and oflags needs to be passed as described above.

Testing

• ci
• We need breaking change to be verified on real world hardware.

[acassis]

Please fix the Documentation as well (to use new the new API function parameters):
https://nuttx.apache.org/docs/latest/applications/examples/media/index.html
https://nuttx.apache.org/docs/latest/components/drivers/special/mtd.html

[xiaoxiang781216]

Please fix the Documentation as well (to use new the new API function parameters): https://nuttx.apache.org/docs/latest/

removed.

applications/examples/media/index.html https://nuttx.apache.org/docs/latest/components/drivers/special/mtd.html

keep as before since it describe the legacy method still work.

[cederom]

Thank you @xiaoxiang781216 :-)

[cederom]

Ah sorry, @acassis is right:

• We are removing wrapping function and replacing it with internal function that has different API.

• I understand this comes from bigger change in MTD, but doc should stay in sync with code and we should put some remark on the API change and give working example.

• This https://nuttx.apache.org/docs/latest/applications/examples/media/index.html states:

  MTD drivers need an additional wrapper layer, the FTL wrapper must first be used to convert the MTD driver to a block device:
  int ret = ftl_initialize(<N>, mtd);
ret = bchdev_register(/dev/mtdblock<N>, <path-to-character-driver>, false);

  While the new API would be: ftl_initialize(FAR const char *path, FAR struct mtd_dev_s *mtd, int oflags), so the example will looks something like this?

  int ret = ftl_initialize(/dev/mtdblock<N>, mtd, O_RDWR);
  ret = bchdev_register(/dev/mtdblock<N>, <path-to-character-driver>, false);
  

• What do you think @mlyszczek @jingfei195887 ? :-)

[mlyszczek]

Well, I don't really feel in the position to comment on such changes, but since you've asked ;)

This is public function so it will probably break someone's code. But change is rather minor and it will not silently break anyone's code (such case should never ever happen). So if this is documented in "upgrade" section of documentation with explanation how to properly migrate, I think this should be fine, as it's rather quick fix.

I would consider adding very short info after compilation fails, that breakage might be caused by API change and that user should refer to documentation (link to page). Maybe even lock it behind kconfig like CONFIG_NUTTX_UPGRADE_12_10_READ, so user can silent warning after he confirms there is no more API breakages? It could be different mechanism, but I think if nuttx changes API, there should be some explicit info for the user that upgrades versions that API has been changed.

[xiaoxiang781216]

I update the document and add the breaking change mark, please review again, @acassis @cederom @mlyszczek .

[Laczen]

Please wait until it is clear how multiple ftl (simple, dhara. ...) are going to be supported.

See #16789.

[xiaoxiang781216]

Please wait until it is clear how multiple ftl (simple, dhara. ...) are going to be supported.

See #16789.

The rename doesn't impact multiple FTL support.

[Laczen]

Please wait until it is clear how multiple ftl (simple, dhara. ...) are going to be supported.
See #16789.

The rename doesn't impact multiple FTL support.

Maybe not, but if the result of multiple FTL support is that both ftl_initialize and ftl_initialize_by_path are needed/useful this PR would need to be undone.

[xiaoxiang781216]

Please wait until it is clear how multiple ftl (simple, dhara. ...) are going to be supported.
See #16789.

The rename doesn't impact multiple FTL support.

Maybe not, but if the result of multiple FTL support is that both ftl_initialize and ftl_initialize_by_path are needed/useful this PR would need to be undone.

ftl_initialize_by_path is more functionality than ftl_initialize, so all ftl_initialize could switch to call ftl_initialize_by_path without problem. Why do you think multiple FTL need the old ftl_initialize?

[Laczen]

Please wait until it is clear how multiple ftl (simple, dhara. ...) are going to be supported.
See #16789.

The rename doesn't impact multiple FTL support.

Maybe not, but if the result of multiple FTL support is that both ftl_initialize and ftl_initialize_by_path are needed/useful this PR would need to be undone.

ftl_initialize_by_path is more functionality than ftl_initialize, so all ftl_initialize could switch to call ftl_initialize_by_path without problem. Why do you think multiple FTL need the old ftl_initialize?

Consistency, if there are other device drivers that are using the xxx_initialize(yyy, minor, ...) (which I think is used e.g. for /dev/eth) it would be better to keep this pattern. This of course would only be valid if it is needed to keep the old ftl_initialize.

[cederom]

Okay, I can understand rationale behind this change, it simplifies stuff, but it is also breaking change as noted by several people, so I marked PR with [BREAKING] tag do it is clearly visible in the PR log, and I have updated PR description on what changes and how (this description also should land into git commit message).

We may want to wait until #16789 is resolved, true, just in case the "by path" is still desired and we do not have to revert anything we would leave stuff as-is :-)

[Laczen]

After going through some other examples I am convinced that the proposal using a path instead of a minor is the correct approach. This also decouples this PR from #16789 as this will be modified to also use a path instead of a minor.

[acassis]

@cederom please review again

[cederom]

This is breaking change so we should keep [BREAKING] mark both in PR title and commit title so people will easily find it in logs and release messages when they find FTL break in their code, please add this tag to PR and commit title, thanks :-)

[acassis]

@cederom I think we don't need to add [BREAKING] in the commit, just the Label is enough. Adding it in the commit passes a wrong message to anyone reading it later.

[cederom]

This is the point of [BREAKING] tag, we discussed it at start of year, there is nothing bad in putting this tag, there is no bad message, it helps quickly finding breaking changes in the git log and release notes (created from PR topic).

Putting it on github label only does not help, as it is not noted in the GIT LOG which is most important (first search), then release notes (based on PR title).

Please start using this tag in the pr topics and git commit topics these are most important. No one later looks into github, there are people who avoid github. GIT LOG is most important + release notes :-)

[cederom]

Look its not a problem of avoiding breaking changes, we need them sometimes, and cannot avoid them. But we should not hide them. Quite opposite we should expose them, even small breaks.

Here we change API, replace functions, removing functions, changing their parameters and functionality. It is important I understand and it helps. But this is breaking change, so we should clearly mark it.

Then someone says hey my function worked different way, they go git log --oneline | grep BREAKING or git log --grep=BREAKING and find okay there was change in the ftl so I need to update my code. As simple as that :-)

[cederom]

I am okay with this code change, no drama ot voting is required as concerns we discussed already, I just want us to follow rule 1.13.9 of https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md in all processed pr/commits please :-)

[cederom]

@jerpelea the release notes use github tag to distinguish breaking changes right?

[acassis]

@cederom I think adding [BREAKING] will deviates from conventional standard like used by other similar projects like Linux kernel and it will make the title cluttered, it should fit to 50 characters.

I suggest adding "BREAKING CHANGE:" in the commit foot, this way will work better for humans and automatic searching scripts

[acassis]

@cederom please let us to follow the https://www.conventionalcommits.org/en/v1.0.0/ it will simplify use of NuttX commits with standardized tools

[cederom]

Do you as you please. This requirement is already in Contributing Guide for over 3 months and everyone seems to ignore it :-(

[cederom]

@cederom please let us to follow the https://www.conventionalcommits.org/en/v1.0.0/ it will simplify use of NuttX commits with standardized tools

Okay we may discuss this approach on the dev mailing list :-) The "!" mark before ":" in the git commit topic seems shorter. Then we can have "BREAKING CHANGE" in the description footer with optional one sentence summary (not sure why is that is the commit message already contains that).

Still I think "[BREAKING]" in the git commit topic prefix and PR title is more clear and visible :-P

[acassis]

@cederom please let us to follow the https://www.conventionalcommits.org/en/v1.0.0/ it will simplify use of NuttX commits with standardized tools

Okay we may discuss this approach on the dev mailing list :-) The "!" mark before ":" in the git commit topic seems shorter. Then we can have "BREAKING CHANGE" in the description footer with optional one sentence summary (not sure why is that is the commit message already contains that).

Still I think "[BREAKING]" in the git commit topic prefix and PR title is more clear and visible :-P

I think following the conventional commits standard will be beneficial to the project. I will submit a PR with the proposal modification and call a discussion in the mailing list.

[Laczen]

It has been more than a month, @cederom please revisit.

[jerpelea]

the commit introduces an compatibility breaking change and PR and commit are not marked as breaking change.
We should consider that community may use the old function name and will create issues for them.
I see 2 options

• mark both the PR and commit so that downstream developers are made aware of the change
• leave the functions as they are and mark 1 function as deprecated (to be removed from NuttX 13.0.0)

[linguini1]

This is a breaking change.

We need breaking change to be verified on real world hardware.

As indicated by Xiang here, it needs hardware testing. Merging must wait for this.

[acassis]

@xiaoxiang781216 any update on hardware testing about this breaking change?

[xiaoxiang781216]

@xiaoxiang781216 any update on hardware testing about this breaking change?

@acassis but this patch just rename the function, no any functionality change.