public inbox for ecos-maintainers@sourceware.org
 help / color / mirror / Atom feed
From: Jonathan Larmour <jifl@jifvik.org>
To: Bart Veer <bartv@ecoscentric.com>
Cc: john@dallaway.org.uk, ecos-maintainers@ecos.sourceware.org
Subject: Re: Flash subsystem update
Date: Fri, 20 Feb 2009 21:22:00 -0000	[thread overview]
Message-ID: <499F1EC0.9030802@jifvik.org> (raw)
In-Reply-To: <pnab8hgtew.fsf@delenn.bartv.net>

Bart Veer wrote:
>>>>>>"Jifl" == Jonathan Larmour <jifl@eCosCentric.com> 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...

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

  reply	other threads:[~2009-02-20 21:22 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-02-11  9:07 eCos 3.0 beta 1 punch list #2 John Dallaway
2009-02-13 19:56 ` Bart Veer
2009-02-17  1:29   ` Jonathan Larmour
2009-02-17  9:18     ` Jonathan Larmour
2009-02-17  9:34     ` Flash subsystem update [ was Re: eCos 3.0 beta 1 punch list #2 ] John Dallaway
2009-02-17 22:01       ` Flash subsystem update John Dallaway
2009-02-18 12:47         ` Bart Veer
2009-02-19  0:08           ` Jonathan Larmour
2009-02-19 11:50             ` Bart Veer
2009-02-19 14:40               ` John Dallaway
2009-02-19 15:13                 ` Bart Veer
2009-02-19 20:53                   ` Jonathan Larmour
2009-02-20  9:16                 ` John Dallaway
2009-02-19 20:31               ` Jonathan Larmour
2009-02-20 12:55                 ` Bart Veer
2009-02-20 21:22                   ` Jonathan Larmour [this message]
2009-02-17 21:07     ` eCos 3.0 beta 1 punch list #2 Bart Veer
2009-02-17 23:10       ` Jonathan Larmour
2009-02-19  0:48 ` Jonathan Larmour

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=499F1EC0.9030802@jifvik.org \
    --to=jifl@jifvik.org \
    --cc=bartv@ecoscentric.com \
    --cc=ecos-maintainers@ecos.sourceware.org \
    --cc=john@dallaway.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).