This is the mail archive of the mailing list for the eCos project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Flash subsystem update

Bart Veer wrote:
"Jifl" == Jonathan Larmour <> writes:

    Jifl> Then how can you retain the semantics of setting the printf
    Jifl> function in cyg_flash_init if the API for doing so is
    Jifl> deprecated? What is the point of calling a new API official,
    Jifl> telling people to start coding to it, and deprecating it
    Jifl> immediately?

It is not a new API. It has been around for some years now. If it was
a completely new API I would not worry about it.

Hardly usably in flashv2-branch.

    >> It has also broken API compatibility for every application that
    >> has used the V2 flash API since that was merged into the
    >> anoncvs trunk.

    Jifl> That is measured in days, and the sole object of the import
    Jifl> was in order to create eCos 3.0. Calling that tiny window
    Jifl> significant for the purposes of backward compatibility
    Jifl> beggars belief. Are we to be compatible with all
    Jifl> intermediate states of the repository?

Days? The flash V2 branch was merged into the trunk over three months
ago, back in November. That started the discussions which led to this
flame war.

Days, not years like the difference to 2.0. It was still the lead-up to 3.0. Otherwise we subvert the point of allowing ongoing development in a CVS repository between stable releases.

    >> It has also broken API compatibility for every eCosPro release
    >> for the last four years or so.

Jifl> I'm only wearing a maintainer hat here. Please do the same.

If the change really gained us anything I would not object. I do not
believe it gains us anything. It does cause an incompatibility problem
for every eCosPro customer over the last 4+ years who wants to upgrade
to a 3.0 sourcebase, as well as some unknown number of anoncvs users.

Bear in mind that one of the specific things we talked about in 3.0 was that, being a point zero release, now was the time to get APIs right, and far more extensive changes were initially proposed. Certainly I don't think many eCos 2.0 apps will be compatible with 3.0.

Incidentally, I raised all this with you in mails in 2004 and 2005. Any compatibility issues were avoidable back then...

Or they can investigate further. When they read the documentation they
would discover that flash subsystem initialization now happens
differently from before, and earlier. They may choose to remove the
cyg_flash_init() call completely. Or they may choose to replace it
with a call to cyg_flash_set_global_printf(). Or they may choose some
other action. Some users may realize that having the flash initialized
earlier means that they can now clean up and simplify some other bits
of their code.

It's not just their code. We can't simplify some of our code either. We could otherwise dispense with the CYG_FLASH_ERR_NOT_INIT checks and associated static(s) for example, because eCosPro users (which seems to be your main concern) could be affected.

The key point is that users get all the information they need to make
an intelligent decision on how to proceed. This contrasts with your
approach of defining cyg_flash_init() to be a no-op. That means that
application code will continue to have a cyg_flash_init() invocation
indefinitely, with no warnings and no indications of any kind that
this no longer has anything to do with flash initialization.

I guess it could be CYG_FAIL instead, and/or return an error code.

(Digression: there is actually a complication here. For full
compatibility cyg_flash_init() should not return 0, it should return
an error code indicating whether or not the flash subsystem has been
fully initialized. That "fully initialized" is not clearly defined.
For example, if a board has both NOR flash and dataflash, and the NOR
flash initializes correctly, but the dataflash fails to initialize
because it is not properly plugged into the socket, should
cyg_flash_init() return 0 or an error code? I don't know yet what the
correct answer is.

This is of course another flaw of cyg_flash_init.

My preferred solution would be for a deprecated
cyg_flash_init() to always return 0, but for the documentation to
describe this problem and suggest that application code uses the
get_info calls to check what actually happened during initialization.)

Indeed, an error would be returned from get_info if the device was never successfully initialised.

Unfortunately I am away this weekend and don't know yet whether I'll
have internet access. So I may not to be able to respond to any
further postings until late Monday or possibly Tuesday.

Sigh. In the interests of moving forwards, I am going to somewhat roll back my changes. But I will retain the set_*printf functions, and add a note in the documentation to indicate that an argument to cyg_flash_init other than NULL is unsupported, and instead cyg_flash_set_global_printf should always be used. In practice I will leave cyg_flash_init to still accept and act on its argument so existing users are unaffected. This will allow us to deprecate and then remove the function more easily at a later date (a macro can check the argument against NULL, and if NULL is used then constant folding will eliminate overhead).

Patch forthcoming on ecos-patches...

------["The best things in life aren't things."]------      Opinions==mine

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]