public inbox for ecos-maintainers@sourceware.org
 help / color / mirror / Atom feed
* eCos 3.0 beta 1 punch list #2
@ 2009-02-11  9:07 John Dallaway
  2009-02-13 19:56 ` Bart Veer
  2009-02-19  0:48 ` Jonathan Larmour
  0 siblings, 2 replies; 19+ messages in thread
From: John Dallaway @ 2009-02-11  9:07 UTC (permalink / raw)
  To: ecos-maintainers, Jonathan Larmour

eCos Maintainers

The remaining punch list items for eCos 3.0 beta 1 are as follows:

a) Constructor priority re-ordering work. (bartv) [ Thank you for the
check-ins, Bart, is this work complete now? Please confirm. ]

d) Fix/workaround for linking of cxxsupp test for ARM targets using the
old arm-elf toolchain based on gcc-3.2.1. (jifl)

g) Final ConfigTool tweaks. (john) [ Almost complete. ]

h) README.host review/update. (john/jifl) [ Over to Jifl now. ]

i) Review of i386 and MIPS32 linking issues reported previously to
ensure everything is now resolved. (jifl)

j) Investigation and workaround for m68k compiler issue at -O0
-fomit-frame-pointer with CYGDBG_USE_ASSERTS. (jifl)

k) Removal of obsolete package CYGPKG_DEVS_FLASH_SST_39VF400 from
repository. (john)

m) Test of support for multiple releases in ecos-install.tcl. (john)

n) Cygwin-hosted sh-elf toolchain spin to match linux-hosted version
dated 2009-01-21. (jifl)

Jifl, can you report status of the items assigned to you please? It
would also be useful if you could generate a new testfarm tarball from
anonCVS (mkanoncvstar.bash) as a further check that recent changes
haven't broken anything major. Finally, I'm unable to login at
http://farm.ecoscentric.com to review testfarm results at present
(apparently because "john@dallaway.org.uk" is not a member of the
"testfarm" group). Can you sort this out please?

Thank you

John Dallaway
eCos 3.0 release manager

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: eCos 3.0 beta 1 punch list #2
  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-19  0:48 ` Jonathan Larmour
  1 sibling, 1 reply; 19+ messages in thread
From: Bart Veer @ 2009-02-13 19:56 UTC (permalink / raw)
  To: John Dallaway; +Cc: ecos-maintainers

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2857 bytes --]

>>>>> "John" == John Dallaway <john@dallaway.org.uk> writes:

    John> eCos Maintainers
    John> The remaining punch list items for eCos 3.0 beta 1 are as follows:

    John> a) Constructor priority re-ordering work. (bartv) [ Thank
    John> you for the check-ins, Bart, is this work complete now?
    John> Please confirm. ]

In theory there is one more change to go in: updating the flash
subsystem to use a prioritized constructor, and associated API
changes. Effectively this would replace explicit calls of
cyg_flash_init() with automatic initialization via a C++ static
constructor. That should simplify higher-level code since there is no
longer any need to worry about whether or not the flash subsystem has
been initialized. It would also give some code size savings in the
flash subsystem, and in the medium to long term it would allow other
subsystems such as fconfig to be statically initialized as well.

However, after experimenting with various different patches, I have
come to the conclusion that this is not the right time to make the
change. It is not too bad when there is only a single flash device and
everything works, but if there are initialization problems then things
get messy. So instead of making the change now, for all anoncvs
targets and with no possiblity of testing on most of those targets, I
want to do the work in eCosPro first. It can then be merged into a
future anoncvs release, once I am confident that it is not going to
break anything.

Referring back to earlier discussions (see ecos-devel 18 Nov 2008):

1) the jffs2/dataflash problems have already been resolved. Flash
   block devices in devtab, initialized at CYG_INIT_IO, will now
   happen after CYG_INIT_BUS_SPI. There is no longer a risk of
   dataflash operations happening before the SPI bus is ready.

2) the specification of cyg_flash_init(), i.e. whether or not it
   takes a printf() function as argument, is not actually important.
   cyg_flash_init() will become deprecated, and mostly a no-op, when
   the flash subsystem switches to using a prioritized constructor.
   It does not actually matter if a deprecated function takes a
   function pointer as argument.

   That means there are no long-term API concerns either. There are
   functions to be added to the API, but those can wait till later.

So, best to leave the anoncvs flash subsystem in its current state for
now. That means I have finished for 3.0.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: eCos 3.0 beta 1 punch list #2
  2009-02-13 19:56 ` Bart Veer
@ 2009-02-17  1:29   ` Jonathan Larmour
  2009-02-17  9:18     ` Jonathan Larmour
                       ` (2 more replies)
  0 siblings, 3 replies; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-17  1:29 UTC (permalink / raw)
  To: Bart Veer; +Cc: John Dallaway, ecos-maintainers

Bart Veer wrote:
> In theory there is one more change to go in: updating the flash
> subsystem to use a prioritized constructor, and associated API
> changes. Effectively this would replace explicit calls of
> cyg_flash_init() with automatic initialization via a C++ static
> constructor. That should simplify higher-level code since there is no
> longer any need to worry about whether or not the flash subsystem has
> been initialized. It would also give some code size savings in the
> flash subsystem, and in the medium to long term it would allow other
> subsystems such as fconfig to be statically initialized as well.
> 
> However, after experimenting with various different patches, I have
> come to the conclusion that this is not the right time to make the
> change. It is not too bad when there is only a single flash device and
> everything works, but if there are initialization problems then things
> get messy.

I need more detail than "get messy". This is what the dev->init member is 
meant to be for. Uninitialised devices get subsequently ignored, and the 
return value of cyg_flash_init is irrelevant.

As with the spec for CYGBLD_GLOBAL_WARNINGS, you seem to be unilaterally 
moving the goalposts from what had previously been agreed *extremely* late 
in the day.

> So instead of making the change now, for all anoncvs
> targets and with no possiblity of testing on most of those targets, I
> want to do the work in eCosPro first. It can then be merged into a
> future anoncvs release, once I am confident that it is not going to
> break anything.

Why can it not be in both? Why should eCosPro be special with respect to a 
changing API? And incompatible.

I don't know whether to wear an eCosCentric hat which says that eCosPro 
should be aimed at well tested code, rather that a testbed for allegedly 
unstable new code; or wear a maintainer hat and say that what happens in 
eCosPro is not relevant to the public project, and API incompatibilities 
made in such a way as to place eCosPro as "more advanced" is not a 
principle to be encouraged.

> 2) the specification of cyg_flash_init(), i.e. whether or not it
>    takes a printf() function as argument, is not actually important.
>    cyg_flash_init() will become deprecated, and mostly a no-op, when
>    the flash subsystem switches to using a prioritized constructor.
>    It does not actually matter if a deprecated function takes a
>    function pointer as argument.

But you've completely omitted how to properly set the printf function.

>    That means there are no long-term API concerns either. There are
>    functions to be added to the API, but those can wait till later.

I disagree. This means we are issuing an API which we have to instantly 
deprecate and anyone presently coding to that API will encounter breakage 
later.

If the API is wrong we need to fix out before we make a major release. A 
stable flash API was the second most critical change to make for eCos 3.0 
after the header updates. We can't flub it. This is a problem I raised 
with you back in 2006.

Let me know what you consider the problems to be and I will deal with them.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: eCos 3.0 beta 1 punch list #2
  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 21:07     ` eCos 3.0 beta 1 punch list #2 Bart Veer
  2 siblings, 0 replies; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-17  9:18 UTC (permalink / raw)
  To: Bart Veer; +Cc: ecos-maintainers

[ker-snip]

Sorry I sounded a bit harsh there. It was late and I was tired. But I do 
believe that a .0 release should go out with an API which will work in future.

More constructively, until I know what the "messy" bit is, I suggest adding:

externC cyg_flash_printf *cyg_flash_set_printf( const cyg_flashaddr_t 
base, cyg_flash_printf *pf);

which associates the printf function of flash at base 'base' with function 
'pf', returning its previous setting. There would also be:
#define CYG_FLASH_SET_PRINTF_ALL ((cyg_flashaddr_t)-1);

which is used to set all devices' printf functions (and always return NULL).

We can instantly deprecate use of cyg_flash_init with non-NULL argument. 
And if the messiness is too much to deal with after all, we can continue 
with cyg_flash_init, and in future it would be more or less #defined to 
cyg_flash_set_printf.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Flash subsystem update [ was Re: eCos 3.0 beta 1 punch list #2 ]
  2009-02-17  1:29   ` Jonathan Larmour
  2009-02-17  9:18     ` Jonathan Larmour
@ 2009-02-17  9:34     ` John Dallaway
  2009-02-17 22:01       ` Flash subsystem update John Dallaway
  2009-02-17 21:07     ` eCos 3.0 beta 1 punch list #2 Bart Veer
  2 siblings, 1 reply; 19+ messages in thread
From: John Dallaway @ 2009-02-17  9:34 UTC (permalink / raw)
  To: Jonathan Larmour, Bart Veer; +Cc: ecos-maintainers

Hi Jifl and Bart

Regarding the prioritised constructor for Flash and consequent API
changes, Jonathan Larmour wrote:

> If the API is wrong we need to fix out before we make a major release. A
> stable flash API was the second most critical change to make for eCos
> 3.0 after the header updates. We can't flub it. This is a problem I
> raised with you back in 2006.
> 
> Let me know what you consider the problems to be and I will deal with
> them.

and later wrote:

> More constructively, until I know what the "messy" bit is, I suggest
> adding:
> 
> externC cyg_flash_printf *cyg_flash_set_printf( const cyg_flashaddr_t
> base, cyg_flash_printf *pf);
> 
> which associates the printf function of flash at base 'base' with
> function 'pf', returning its previous setting. There would also be:
> #define CYG_FLASH_SET_PRINTF_ALL ((cyg_flashaddr_t)-1);
> 
> which is used to set all devices' printf functions (and always return
> NULL).
> 
> We can instantly deprecate use of cyg_flash_init with non-NULL argument.
> And if the messiness is too much to deal with after all, we can continue
> with cyg_flash_init, and in future it would be more or less #defined to
> cyg_flash_set_printf.

FAOD, I'll take this as a showstopper issue for 3.0 beta 1 on the basis
that we don't want API changes between beta and final. My concerns are:

a) Further slippage to the beta release.

b) The trunk is currently semi-frozen (no large/invasive changes) which
prevents the maintainers from undertaking regular maintainer duties.

The two issues are, of course, related. If we're able to address the
Flash issue this week, I think it makes sense to cut the 3.0 release
branch now (while we have stability) and double commit the Flash API
changes to address issue (b). If the Flash issue will take longer, then
we should hold back on cutting the branch and lift the semi-frozen
status of the repository. We then risk some de-stabilisation.

We need to contain further slippage as far as practically possible and
push forward with eCos 3.0. I would therefore like to ensure we have a
well-understood plan for addressing this issue in a realistic timescale.
Firstly, we need to understand the issues that Bart encountered in
detail. Bart, can you provide this detail today please? Does Jifl's
proposed workaround work for you?

John Dallaway

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: eCos 3.0 beta 1 punch list #2
  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 21:07     ` Bart Veer
  2009-02-17 23:10       ` Jonathan Larmour
  2 siblings, 1 reply; 19+ messages in thread
From: Bart Veer @ 2009-02-17 21:07 UTC (permalink / raw)
  To: Jonathan Larmour; +Cc: ecos-maintainers

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 5965 bytes --]

>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:

    >> So instead of making the change now, for all anoncvs targets
    >> and with no possiblity of testing on most of those targets, I
    >> want to do the work in eCosPro first. It can then be merged
    >> into a future anoncvs release, once I am confident that it is
    >> not going to break anything.

    Jifl> Why can it not be in both? Why should eCosPro be special
    Jifl> with respect to a changing API? And incompatible.

Switching the flash subsystem to initialize via static constructors
means that the h/w init order changes for every platform that uses
RedBoot, and very possibly some of the remaining platforms as well.
Currently flash initialization happens after cyg_start() has started
running. All static constructors have already run. Virtual vectors
have been sorted out. The memory map has been sorted via
HAL_MEM_REAL_REGION_TOP() if necessary. Any bist() self-test code has
run. Some other RedBoot init code may have run as well.

Switching to a static constructor means that the flash initialization
will now happen much earlier, before cyg_start(). Now, it is likely
that on most platforms that won't cause any problems - it worked fine
on the platform I tried. But we may easily have overlooked some
dependency somewhere which means that other platforms will stop
working, so that if anybody decides to rebuild and install RedBoot
after 3.0 they'll end up with a brick.

Rolling out this change in anoncvs means that it will affect every
single target in one fell swoop. Rolling it out in eCosPro means that
we get to test the new code one or a small number of ports at a time,
as we do new ports. Typically we'll make very sure that we have a way
of unbricking any board we are porting to, before we start messing
about with the flash. So if there are problems with the new code, we
can find them and fix them prior to any eCosPro release, with no risk
of affecting either our customers or anoncvs users. Over a period of
time the new code will be run on a variety of hardware with different
flash setups.

What I am proposing is pretty much the same strategy that was adopted
for the flash V2 work. That did not immediately go into the anoncvs
trunk because it was considered too dangerous a change. Instead it
went into a separate branch, for those anoncvs users wanting the new
functionality, and it went into eCosPro. Partly due to the latter, by
now flash V2 has been used on sufficiently many different targets that
we believe there is very little risk of it breaking the world for
typical anoncvs users, so the code has been merged into the trunk.

Now, for most application developers this is a storm in a teacup. It
really does not matter all that much whether the flash
auto-initializes or whether they need to make an extra init call from
inside their application.

So, we have a change that offers very little benefit to users, which
significantly changes the order in which nearly all targets
initialize, which may cause some number of targets to break and leave
users with bricks, being put into the source tree very close to
release after not very much testing. The more I worked on this, the
more I realized this was a really bad idea.

>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:

    Jifl> externC cyg_flash_printf *cyg_flash_set_printf( const cyg_flashaddr_t 
    Jifl> base, cyg_flash_printf *pf);

    Jifl> which associates the printf function of flash at base 'base'
    Jifl> with function 'pf', returning its previous setting. There
    Jifl> would also be: #define CYG_FLASH_SET_PRINTF_ALL
    Jifl> ((cyg_flashaddr_t)-1);

    Jifl> which is used to set all devices' printf functions (and
    Jifl> always return NULL).

    Jifl> We can instantly deprecate use of cyg_flash_init with
    Jifl> non-NULL argument. And if the messiness is too much to deal
    Jifl> with after all, we can continue with cyg_flash_init, and in
    Jifl> future it would be more or less #defined to
    Jifl> cyg_flash_set_printf.

The discussion on all this back in November involved an incompatible
API change, changing cyg_flash_init() so that it no longer took any
arguments. That meant we had to add new functions for setting the
printf function, e.g. cyg_flash_set_global_printf() and
cyg_flash_set_printf(). 

Now, there are no longer any plans to change the prototype of
cyg_flash_init(). It will continue to take a printf function, and act
as cyg_flash_set_global_printf(). However we'll probably want to mark
it as deprecated to make sure that people are aware things are done a
bit differently than before.

Since for 3.0 it will still be necessary for applications to call
cyg_flash_init(), and since cyg_flash_init() still takes a printf
pointer, there is very little point in implementing the set_printf()
functions right now. It would only affect application code that
currently calls cyg_flash_init() multiple times with different
arguments, to change the printf function at run-time. I suspect that
very few applications do that. If they do then in future they'll get
multiple compile-time warnings about cyg_flash_init() being
deprecated, as opposed to just a single warning about cyg_flash_init()
and no warnings about the cyg_flash_set_global_printf() calls.

So, a change that is likely to benefit very few users, and negligible
future harm if we leave the API as it is right now. Again, the
most sensible thing to do is to leave things as they are and come back
to this after the release.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  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       ` John Dallaway
  2009-02-18 12:47         ` Bart Veer
  0 siblings, 1 reply; 19+ messages in thread
From: John Dallaway @ 2009-02-17 22:01 UTC (permalink / raw)
  To: Jonathan Larmour, Bart Veer; +Cc: ecos-maintainers

Hi Jifl and Bart

John Dallaway wrote:

> We need to contain further slippage as far as practically possible and
> push forward with eCos 3.0. I would therefore like to ensure we have a
> well-understood plan for addressing this issue in a realistic timescale.
> Firstly, we need to understand the issues that Bart encountered in
> detail. Bart, can you provide this detail today please? Does Jifl's
> proposed workaround work for you?

Bart, thank you for the details of the technical issues.

Jifl, I think Bart makes some valid points regarding a cautious roll out
of any revised Flash API. I also appreciate your own perspective
regarding adoption the new API as part of the new major version of eCos.

Personally, I would prefer that we seize this opportunity to switch API
rather than wait for the next major release of eCos beyond 3.0. If we
switch now, the onus would be on the eCos community to deliver a verdict
on the changes through diligent testing over the beta period and I would
hope that the eCos maintainers could commit to verify RedBoot across a
diverse sample of the boards we have to hand. We do have the option to
extend the beta period, or push out a second beta, if it proves necessary.

Jifl, what are your thoughts on all this?

John Dallaway

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: eCos 3.0 beta 1 punch list #2
  2009-02-17 21:07     ` eCos 3.0 beta 1 punch list #2 Bart Veer
@ 2009-02-17 23:10       ` Jonathan Larmour
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-17 23:10 UTC (permalink / raw)
  To: Bart Veer; +Cc: ecos-maintainers

Bart Veer wrote:
>>>>>>"Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:
> 
> 
>     >> So instead of making the change now, for all anoncvs targets
>     >> and with no possiblity of testing on most of those targets, I
>     >> want to do the work in eCosPro first. It can then be merged
>     >> into a future anoncvs release, once I am confident that it is
>     >> not going to break anything.
> 
>     Jifl> Why can it not be in both? Why should eCosPro be special
>     Jifl> with respect to a changing API? And incompatible.
> 
> Switching the flash subsystem to initialize via static constructors
> means that the h/w init order changes for every platform that uses
> RedBoot, and very possibly some of the remaining platforms as well.
> Currently flash initialization happens after cyg_start() has started
> running.  All static constructors have already run. Virtual vectors
> have been sorted out. The memory map has been sorted via
> HAL_MEM_REAL_REGION_TOP() if necessary. Any bist() self-test code has
> run. Some other RedBoot init code may have run as well.

Not always. For example if you build JFFS2 into RedBoot, as people now not 
infrequently do, cyg_flash_init gets called at priority CYG_INIT_IO. We 
have not had any problems with this reported by ecospro or anoncvs users. 
Certainly while it's possible to construct straw men that could cause 
problems, I think we need something rather more concrete as an argument 
against fixing the API.

Incidentally, you said the messiness was to do with initialising multiple 
flash devices, which wouldn't apply for most ports anyway as they will use 
the legacy device API which only supports a single device.

> Switching to a static constructor means that the flash initialization
> will now happen much earlier, before cyg_start(). Now, it is likely
> that on most platforms that won't cause any problems - it worked fine
> on the platform I tried. But we may easily have overlooked some
> dependency somewhere which means that other platforms will stop
> working, so that if anybody decides to rebuild and install RedBoot
> after 3.0 they'll end up with a brick.

There are all sorts of factors with many pieces of code that have changed 
substantially: redboot (particularly its flash code) which is far from 
identical with eCosPro, device drivers, hal/common, HALs, v2 flash 
drivers, legacy mode in v2 io/flash (not tested by ecospro certainly), 
compiler differences, documentation which may have bit-rotted. This seems 
relatively minor compared with some of those. Your concern is the 
rationale for the (now de facto obsolete) ecos/images hierarchy - known 
good images. We can't promise anything otherwise - it's just best effort. 
I see nothing especially different here than changes in any of those other 
packages which could also easily cause targets to be bricked.

> Rolling out this change in anoncvs means that it will affect every
> single target in one fell swoop. Rolling it out in eCosPro means that
> we get to test the new code one or a small number of ports at a time,
> as we do new ports. Typically we'll make very sure that we have a way
> of unbricking any board we are porting to, before we start messing
> about with the flash. So if there are problems with the new code, we
> can find them and fix them prior to any eCosPro release, with no risk
> of affecting either our customers or anoncvs users. Over a period of
> time the new code will be run on a variety of hardware with different
> flash setups.

Meanwhile eCos 3.0 users are left with a deprecated obsolete API. How 
eCosCentric chooses to roll this out for their customers is not my concern 
at the moment. It's important to get the API right for a point zero release.

> What I am proposing is pretty much the same strategy that was adopted
> for the flash V2 work. That did not immediately go into the anoncvs
> trunk because it was considered too dangerous a change.  Instead it
> went into a separate branch, for those anoncvs users wanting the new
> functionality, and it went into eCosPro.

That seems rather revisionist. It went into eCosPro because eCosCentric 
wanted the functionality, not because of some altruistic notion of testing 
things in advance for public eCos users. It was developed in a branch 
because it was developed progressively in it, rather than being a big-bang 
replacement per se.

> Now, for most application developers this is a storm in a teacup. It
> really does not matter all that much whether the flash
> auto-initializes or whether they need to make an extra init call from
> inside their application.

What matters is whether we care a jot about compatibility with documented 
APIs. If we are going to say that code written against published APIs may 
not work with eCos 3.0.1 or eCos 3.1 or whatever with the same semantics, 
then sure we should go ahead.

> So, we have a change that offers very little benefit to users, which
> significantly changes the order in which nearly all targets
> initialize, which may cause some number of targets to break and leave
> users with bricks, being put into the source tree very close to
> release after not very much testing. The more I worked on this, the
> more I realized this was a really bad idea.

That's what the time between beta and final is for. And after 3.0 we can 
look for a minor bug fix 3.0.1 release. Even if your paranoia turned out 
to be justified, we could just start shipping a 3.0.1.

> The discussion on all this back in November involved an incompatible
> API change, changing cyg_flash_init() so that it no longer took any
> arguments. That meant we had to add new functions for setting the
> printf function, e.g. cyg_flash_set_global_printf() and
> cyg_flash_set_printf(). 

Sure, those names are fine too. I was in a rush this morning. Maybe the 
naming was even my idea before - haven't looked.

> Now, there are no longer any plans to change the prototype of
> cyg_flash_init().  It will continue to take a printf function, and act
> as cyg_flash_set_global_printf(). However we'll probably want to mark
> it as deprecated to make sure that people are aware things are done a
> bit differently than before.

But without a cyg_flash_set_global_printf() people can't write code that 
will continue working in the same way from one release to the next (i.e. 
API breakage).

> Since for 3.0 it will still be necessary for applications to call
> cyg_flash_init(), and since cyg_flash_init() still takes a printf
> pointer, there is very little point in implementing the set_printf()
> functions right now.

You are conflating two problems - initialisation and the printf function. 
We can solve the printf problem separately from the initialisation one. 
For 3.0 we /could/ say that calling cyg_flash_init with a non-NULL 
argument is deprecated. The printf function should be set with the 
set_printf() function. With the initialisation fixed, cyg_flash_init could 
then be #defined to something like

#define cyg_flash_init(pf) do { if (pf) cyg_flash_set_global_printf(pf); } 
while (0)

leaving no overhead (with an optimising compiler) for those who called it 
with NULL. We could leave removing it till later. But I'd rather just 
remove it now and say that the printf function can only be set by 
set_(global)_printf and the argument to cyg_flash_init is ignored.

This would be sufficient to prevent API breakage.

But stepping further back, I'd rather just sort out the initialisation 
problem anyway and get it out of the way. I see the risks as small given 
the facts about initialisation dependencies. It's not like this is the 
final and won't be tested before release - we will be actively seeking 
testers and I bet we'll get them. I can give you an I Told You So card, 
which will oblige me to buy you a Wrestlers lunch if there is any bricking 
caused by the initialisation order.

> It would only affect application code that
> currently calls cyg_flash_init() multiple times with different
> arguments, to change the printf function at run-time.
> I suspect that very few applications do that.

Any program using JFFS2 already does this. The ability to change the 
printf function was specifically added by contributors.

BTW, you still haven't clarified what the messy bit is with initialising 
multiple devices.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["The best things in life aren't things."]------      Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-17 22:01       ` Flash subsystem update John Dallaway
@ 2009-02-18 12:47         ` Bart Veer
  2009-02-19  0:08           ` Jonathan Larmour
  0 siblings, 1 reply; 19+ messages in thread
From: Bart Veer @ 2009-02-18 12:47 UTC (permalink / raw)
  To: John Dallaway; +Cc: ecos-maintainers

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3614 bytes --]

>>>>> "John" == John Dallaway <john@dallaway.org.uk> writes:

    John> Hi Jifl and Bart
    John> John Dallaway wrote:

    >> We need to contain further slippage as far as practically
    >> possible and push forward with eCos 3.0. I would therefore like
    >> to ensure we have a well-understood plan for addressing this
    >> issue in a realistic timescale. Firstly, we need to understand
    >> the issues that Bart encountered in detail. Bart, can you
    >> provide this detail today please? Does Jifl's proposed
    >> workaround work for you?

    John> Bart, thank you for the details of the technical issues.

    John> Jifl, I think Bart makes some valid points regarding a
    John> cautious roll out of any revised Flash API. I also
    John> appreciate your own perspective regarding adoption the new
    John> API as part of the new major version of eCos.

    John> Personally, I would prefer that we seize this opportunity to
    John> switch API rather than wait for the next major release of
    John> eCos beyond 3.0. If we switch now, the onus would be on the
    John> eCos community to deliver a verdict on the changes through
    John> diligent testing over the beta period and I would hope that
    John> the eCos maintainers could commit to verify RedBoot across a
    John> diverse sample of the boards we have to hand. We do have the
    John> option to extend the beta period, or push out a second beta,
    John> if it proves necessary.

I am not sure why we are still discussing this.

By far the most sensible thing to do right now is to leave things
exactly as they are. The functionality change is of interest to very
few if any users. It may be interesting to us in the long term if we
want to statically initialize more subsystems, but that does not
require anything to be done today. There are no significant API
compatibility implications: all of the functions that exist today will
continue to exist; one of them will become pretty much irrelevant and
users will be warned that they can safely remove it and save a few
bytes, but it will continue to exist.

If things go wrong, the result may be not a simple test failure. It
may be a bricked board, and not everybody will have facilities for
unbricking. I have had enough experience with different boards over
the years to know that getting the flash working right may be tricky
and that there may be subtle interactions between subsystems.

So, I don't see an upside for changing anoncvs now. The downside is,
at best, delaying the release further and, at worst, breaking things
completely for some ports and users ending up with bricked boards.
There is a sensible way forward to introduce and test the changes
gradually, rather than forcing it for all 116 anoncvs targets in one
go with minimal testing. From my perspective it is a no-brainer.

I have far too many other things to get on with right now, so I have
no interest in discussing this any further. I do not intend to return
to this subject until after the 3.0 release. If I do find some more
time to spend on eCos 3.0 it will be used for something that actually
matters, e.g. sorting out the C++ support for the synthetic target.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-18 12:47         ` Bart Veer
@ 2009-02-19  0:08           ` Jonathan Larmour
  2009-02-19 11:50             ` Bart Veer
  0 siblings, 1 reply; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-19  0:08 UTC (permalink / raw)
  To: Bart Veer; +Cc: John Dallaway, ecos-maintainers

Bart Veer wrote:
> 
> By far the most sensible thing to do right now is to leave things
> exactly as they are. The functionality change is of interest to very
> few if any users. It may be interesting to us in the long term if we
> want to statically initialize more subsystems, but that does not
> require anything to be done today. There are no significant API
> compatibility implications: all of the functions that exist today will
> continue to exist; one of them will become pretty much irrelevant and
> users will be warned that they can safely remove it and save a few
> bytes, but it will continue to exist.

I still think you haven't got the point of what is being discussed. As 
I've mentioned a few times now, your concerns are to do with initialising 
by C++ constructor. While I think the risks of this change are rather 
theoretical, the effects on setting the printf function are not, and 
that's the source of API breakage when cyg_flash_init disappears in future.

Since you aren't keen to work on 3.0 any more or discuss further, I'm 
checking in a patch which updates cyg_flash_init to remove its argument (I 
toyed with keeping the argument and deprecating it but that seemed the 
worst of all worlds by hiding the change for any existing API users). And 
the main benefit of doing so is that later on we can do:
#define cyg_flash_init() CYG_EMPTY_STATEMENT
and there's no overhead, and no API breakage.

The patch adds the functions proposed by your good self on 18th Nov and 
updates everything accordingly, including docs. See ecos-patches.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: eCos 3.0 beta 1 punch list #2
  2009-02-11  9:07 eCos 3.0 beta 1 punch list #2 John Dallaway
  2009-02-13 19:56 ` Bart Veer
@ 2009-02-19  0:48 ` Jonathan Larmour
  1 sibling, 0 replies; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-19  0:48 UTC (permalink / raw)
  To: John Dallaway; +Cc: ecos-maintainers

John Dallaway wrote:
> 
> d) Fix/workaround for linking of cxxsupp test for ARM targets using the
> old arm-elf toolchain based on gcc-3.2.1. (jifl)

FTR, done.

> h) README.host review/update. (john/jifl) [ Over to Jifl now. ]

I've reviewed what you sent on 2009-02-13. Looks spot on.

> i) Review of i386 and MIPS32 linking issues reported previously to
> ensure everything is now resolved. (jifl)

I confess I've lost the state on this a bit. Have you already checked they 
now link? If so then there's no need for me to duplicate that check.

> j) Investigation and workaround for m68k compiler issue at -O0
> -fomit-frame-pointer with CYGDBG_USE_ASSERTS. (jifl)

README will do for beta.  Should be looked at more before final though.

> n) Cygwin-hosted sh-elf toolchain spin to match linux-hosted version
> dated 2009-01-21. (jifl)

FTR done.

> Jifl, can you report status of the items assigned to you please? It
> would also be useful if you could generate a new testfarm tarball from
> anonCVS (mkanoncvstar.bash) as a further check that recent changes
> haven't broken anything major.

Done just now.

> Finally, I'm unable to login at
> http://farm.ecoscentric.com to review testfarm results at present
> (apparently because "john@dallaway.org.uk" is not a member of the
> "testfarm" group). Can you sort this out please?

As Alex may or may not have told you separately it only looked like that 
because the page wasn't customised for those in the 'testresults' group. 
Going direct to https://farm.ecoscentric.com/tfq.cgi would have worked, 
but Alex has now customised http://farm.ecoscentric.com anyway so it 
should all be hunkydory.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-19  0:08           ` Jonathan Larmour
@ 2009-02-19 11:50             ` Bart Veer
  2009-02-19 14:40               ` John Dallaway
  2009-02-19 20:31               ` Jonathan Larmour
  0 siblings, 2 replies; 19+ messages in thread
From: Bart Veer @ 2009-02-19 11:50 UTC (permalink / raw)
  To: Jonathan Larmour; +Cc: john, ecos-maintainers

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4160 bytes --]

>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:

    Jifl> Bart Veer wrote:
    >> 
    >> By far the most sensible thing to do right now is to leave
    >> things exactly as they are. The functionality change is of
    >> interest to very few if any users. It may be interesting to us
    >> in the long term if we want to statically initialize more
    >> subsystems, but that does not require anything to be done
    >> today. There are no significant API compatibility implications:
    >> all of the functions that exist today will continue to exist;
    >> one of them will become pretty much irrelevant and users will
    >> be warned that they can safely remove it and save a few bytes,
    >> but it will continue to exist.

    Jifl> I still think you haven't got the point of what is being
    Jifl> discussed. As I've mentioned a few times now, your concerns
    Jifl> are to do with initialising by C++ constructor. While I
    Jifl> think the risks of this change are rather theoretical, the
    Jifl> effects on setting the printf function are not, and that's
    Jifl> the source of API breakage when cyg_flash_init disappears in
    Jifl> future.

Uh, excuse me. If you look again at the email I sent last Friday, I
clearly discussed both the constructor priority issue and the API
issue. I concluded that keeping things exactly as they were did not
involve any API compatibility issues.
    
    Jifl> Since you aren't keen to work on 3.0 any more or discuss
    Jifl> further, I'm checking in a patch which updates
    Jifl> cyg_flash_init to remove its argument (I toyed with keeping
    Jifl> the argument and deprecating it but that seemed the worst of
    Jifl> all worlds by hiding the change for any existing API users).
    Jifl> And the main benefit of doing so is that later on we can do:
    Jifl> #define cyg_flash_init() CYG_EMPTY_STATEMENT and there's no
    Jifl> overhead, and no API breakage.

    Jifl> The patch adds the functions proposed by your good self on
    Jifl> 18th Nov and updates everything accordingly, including docs.
    Jifl> See ecos-patches.

This is exactly the wrong thing to do, as I figured out after 18 Nov
when I actually started making changes along these lines. As I did the
work I realized that the changes were both unnecessary and harmful.

The correct thing to do in a future release is to keep
cyg_flash_init() exactly as it was before your check-in, but marked
deprecated.

  __externC int cyg_flash_init( cyg_flash_printf *pf ) __attribute__ ((deprecated));

or even better, if gcc were to support it:

  __externC int cyg_flash_init( cyg_flash_printf *pf )
      __attribute__ ((deprecated ("explicit calls to cyg_flash_init() are no longer required")));

cyg_flash_init() would continue to exist indefinitely and would
continue to take a printf function pointer. There is no API breakage.
However people who bother to look at the compiler warnings would
figure out that they could save a few bytes of code.

Your change to cyg_flash_init() has broken API compatibility for every
flash-using application that has used the V2 flash branch since it was
created. It has also broken API compatibility for every application
that has used the V2 flash API since that was merged into the anoncvs
trunk. It has also broken API compatibility for every eCosPro release
for the last four years or so. And it does not gain us anything. That
is why I abandoned my work along these lines.

I want to see this change reverted. You can keep the
cyg_flash_set_printf() and cyg_flash_set_global_printf() functions if
you really want to. They don't add any significant value, but they are
also harmless. However cyg_flash_init() should go back the way it was.

Bart
    
-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-19 11:50             ` Bart Veer
@ 2009-02-19 14:40               ` John Dallaway
  2009-02-19 15:13                 ` Bart Veer
  2009-02-20  9:16                 ` John Dallaway
  2009-02-19 20:31               ` Jonathan Larmour
  1 sibling, 2 replies; 19+ messages in thread
From: John Dallaway @ 2009-02-19 14:40 UTC (permalink / raw)
  To: Bart Veer, Jonathan Larmour; +Cc: ecos-maintainers

Hi Jifl and Bart

Jonathan Larmour wrote:

> ... I'm checking in a patch which updates cyg_flash_init to remove
> its argument (I toyed with keeping the argument and deprecating it
> but that seemed the worst of all worlds by hiding the change for any
> existing API users). And the main benefit of doing so is that later
> on we can do:
> #define cyg_flash_init() CYG_EMPTY_STATEMENT
> and there's no overhead, and no API breakage.

and Bart Veer replied:

> Your change to cyg_flash_init() has broken API compatibility for every
> flash-using application that has used the V2 flash branch since it was
> created. It has also broken API compatibility for every application
> that has used the V2 flash API since that was merged into the anoncvs
> trunk. It has also broken API compatibility for every eCosPro release
> for the last four years or so.

So it seems you have opposite perspectives on whether it is preferable to:

a) preserve API compatibility when the flash subsystem switches over to
using a prioritized constructor, leaving legacy support code in place
for the foreseeable future

or

b) break API compatibility now, drawing the attention of developers to
the forthcoming change and providing a route to the complete elimination
of deprecated code in the future

There is no "right answer" here, but I would note that:

* A major new release of the code is the best time to make
forward-looking API changes

* As things stand, breakage to existing application builds will be
trivial to fix at the application level

I suggest we give the other eCos maintainers an opportunity to comment
today. I will defer branching for eCos 3.0 until tomorrow morning.

John Dallaway
eCos 3.0 release manager

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  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
  1 sibling, 1 reply; 19+ messages in thread
From: Bart Veer @ 2009-02-19 15:13 UTC (permalink / raw)
  To: John Dallaway; +Cc: jifl, ecos-maintainers

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3072 bytes --]

>>>>> "John" == John Dallaway <john@dallaway.org.uk> writes:

    John> Hi Jifl and Bart
    John> Jonathan Larmour wrote:

    >> ... I'm checking in a patch which updates cyg_flash_init to remove
    >> its argument (I toyed with keeping the argument and deprecating it
    >> but that seemed the worst of all worlds by hiding the change for any
    >> existing API users). And the main benefit of doing so is that later
    >> on we can do:
    >> #define cyg_flash_init() CYG_EMPTY_STATEMENT
    >> and there's no overhead, and no API breakage.

    John> and Bart Veer replied:

    >> Your change to cyg_flash_init() has broken API compatibility
    >> for every flash-using application that has used the V2 flash
    >> branch since it was created. It has also broken API
    >> compatibility for every application that has used the V2 flash
    >> API since that was merged into the anoncvs trunk. It has also
    >> broken API compatibility for every eCosPro release for the last
    >> four years or so.

    John> So it seems you have opposite perspectives on whether it is
    John> preferable to:

    John> a) preserve API compatibility when the flash subsystem
    John> switches over to using a prioritized constructor, leaving
    John> legacy support code in place for the foreseeable future

    John> or

    John> b) break API compatibility now, drawing the attention of
    John> developers to the forthcoming change and providing a route
    John> to the complete elimination of deprecated code in the future

    John> There is no "right answer" here, but I would note that:

    John> * A major new release of the code is the best time to make
    John> forward-looking API changes

    John> * As things stand, breakage to existing application builds will be
    John> trivial to fix at the application level

    John> I suggest we give the other eCos maintainers an opportunity
    John> to comment today. I will defer branching for eCos 3.0 until
    John> tomorrow morning.

No, you still don't understand. My original proposal NEVER breaks
compatibility. Not now, because it involved zero changes to the API.
Not in future, because there will always be a cyg_flash_init() with
the exact same interface as it does now. It will become deprecated but
it will still exist.

Jifl's patches have broken API compatibility with existing code. In
future his modified cyg_flash_init() will still exist but it will be
deprecated in the same way as per my proposal.

So Jifl's patches have gained us absolutely nothing. However they have
broken compatibility for existing applications, and with 4+ years of
eCosPro releases.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-19 11:50             ` Bart Veer
  2009-02-19 14:40               ` John Dallaway
@ 2009-02-19 20:31               ` Jonathan Larmour
  2009-02-20 12:55                 ` Bart Veer
  1 sibling, 1 reply; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-19 20:31 UTC (permalink / raw)
  To: Bart Veer; +Cc: john, ecos-maintainers

Bart Veer wrote:
>>>>>>"Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:
> 
>     Jifl> I still think you haven't got the point of what is being
>     Jifl> discussed. As I've mentioned a few times now, your concerns
>     Jifl> are to do with initialising by C++ constructor. While I
>     Jifl> think the risks of this change are rather theoretical, the
>     Jifl> effects on setting the printf function are not, and that's
>     Jifl> the source of API breakage when cyg_flash_init disappears in
>     Jifl> future.
> 
> Uh, excuse me. If you look again at the email I sent last Friday, I
> clearly discussed both the constructor priority issue and the API
> issue. I concluded that keeping things exactly as they were did not
> involve any API compatibility issues.

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

> The correct thing to do in a future release is to keep
> cyg_flash_init() exactly as it was before your check-in, but marked
> deprecated.
[snip]
> cyg_flash_init() would continue to exist indefinitely and would
> continue to take a printf function pointer. There is no API breakage.
> However people who bother to look at the compiler warnings would
> figure out that they could save a few bytes of code.
> 
> Your change to cyg_flash_init() has broken API compatibility for every
> flash-using application that has used the V2 flash branch since it was
> created.

But as you yourself said, the V2 flash branch is a development branch, and 
rightly so. There has been no release from it, nor has it been 
release-worthy. Anyone working against it faced a large struggle because 
it was interdependent on the rest of the tree which was ancient. And as we 
know from eCosCentric work, a lot of the redboot stuff was broken anyway, 
and we spent a lot of time fixing much of it, which has only gotten 
integrated as part of the merge to trunk.

I have no intention of retaining compatibility with an obsolete -cum- work 
in progress tree. What would be the point of that.

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

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

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

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

The flash v2 API has never been released and there are no backward 
compatibility implications. eCos 3.0 is what cements the API. What I am 
concerned about is forward compatibility. I would rather have the API 
completely straight, including removal of cyg_flash_init. But since you 
are resistant to that, I have compromised with something which at least 
allows it to be #define'd away to nothing.

Before now there hasn't _been_ a cyg_flash_init function to be compatible 
with. And I've ensured that the legacy API's flash_init() DTRT.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-19 15:13                 ` Bart Veer
@ 2009-02-19 20:53                   ` Jonathan Larmour
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-19 20:53 UTC (permalink / raw)
  To: Bart Veer; +Cc: John Dallaway, ecos-maintainers

Bart Veer wrote:
>     John> So it seems you have opposite perspectives on whether it is
>     John> preferable to:
> 
>     John> a) preserve API compatibility when the flash subsystem
>     John> switches over to using a prioritized constructor, leaving
>     John> legacy support code in place for the foreseeable future
> 
>     John> or
> 
>     John> b) break API compatibility now, drawing the attention of
>     John> developers to the forthcoming change and providing a route
>     John> to the complete elimination of deprecated code in the future
> 
>     John> There is no "right answer" here, but I would note that:
> 
>     John> * A major new release of the code is the best time to make
>     John> forward-looking API changes
> 
>     John> * As things stand, breakage to existing application builds will be
>     John> trivial to fix at the application level
> 
>     John> I suggest we give the other eCos maintainers an opportunity
>     John> to comment today. I will defer branching for eCos 3.0 until
>     John> tomorrow morning.
> 
> No, you still don't understand. My original proposal NEVER breaks
> compatibility. Not now, because it involved zero changes to the API.
> Not in future, because there will always be a cyg_flash_init() with
> the exact same interface as it does now. It will become deprecated but
> it will still exist.

Deprecating an API function means requiring API users to stop using it. 
Are you requiring them to change their application, freshly written to 
this new API, or not?

Until my change, the only way to set the printf function was with 
cyg_flash_init. That function rightly should go away, not bloat things in 
perpetuity.

> Jifl's patches have broken API compatibility with existing code. In
> future his modified cyg_flash_init() will still exist but it will be
> deprecated in the same way as per my proposal.

> So Jifl's patches have gained us absolutely nothing. However they have
> broken compatibility for existing applications, and with 4+ years of
> eCosPro releases.

Again, I'm writing solely as a maintainer.

I'd like to know how many applications you think are based on the *public* 
eCos project using flash v2, given its dependency on an ancient tree. I do 
know that people _tried_ to use it. And gave up.

I did think of retaining the existing function signature, but documenting 
a requirement for a NULL argument, but there seemed no point given this 
has never been a proper API. Moreover changing the function signature 
means that should there somehow be incorrect use of cyg_flash_init, it 
will result in a cleanly reported error to the user. Not that I expect 
that to happen outside of eCosCentric (RBL is the only thing I can think of).

The only people affected are eCosCentric, because they/we slightly jumped 
the gun and integrated it in their own trunk rather than making that 
effort in the public sources. With a maintainer's hat on, that's not to do 
with the public project.

Jifl
-- 
*See us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300*
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["Si fractum non sit, noli id reficere"]------       Opinions==mine

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-19 14:40               ` John Dallaway
  2009-02-19 15:13                 ` Bart Veer
@ 2009-02-20  9:16                 ` John Dallaway
  1 sibling, 0 replies; 19+ messages in thread
From: John Dallaway @ 2009-02-20  9:16 UTC (permalink / raw)
  To: Bart Veer; +Cc: ecos-maintainers

Hi Bart

John Dallaway wrote:

> There is no "right answer" here, but I would note that:
> 
> * A major new release of the code is the best time to make
> forward-looking API changes
> 
> * As things stand, breakage to existing application builds will be
> trivial to fix at the application level
> 
> I suggest we give the other eCos maintainers an opportunity to comment
> today. I will defer branching for eCos 3.0 until tomorrow morning.

I've received no comments from anyone else on this issue. As things
stand, I could not justify the backing out of Jifl's patch. I will
therefore proceed with cutting the branch this morning. I appreciate
your objections but there appears to be no middle ground here and we
have to move forward...

John Dallaway

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-19 20:31               ` Jonathan Larmour
@ 2009-02-20 12:55                 ` Bart Veer
  2009-02-20 21:22                   ` Jonathan Larmour
  0 siblings, 1 reply; 19+ messages in thread
From: Bart Veer @ 2009-02-20 12:55 UTC (permalink / raw)
  To: Jonathan Larmour; +Cc: john, ecos-maintainers

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 6440 bytes --]

>>>>> "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.

    >> 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.    
    
    >> 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.

Now, much of the 3.0 work to date has been done on eCosCentric company
time. Some of the remaining 3.0 work also depends on eCosCentric
resources, e.g. access to the company's test farm. I think the company
can quite reasonably feel narked at an unnecessary last-minute
incompatible change that is going to make life more difficult for some
of its customers. I do not expect that to affect the 3.0 effort, this
is not intended as any kind of threat. However, I do believe it will
make it even more difficult in future to get any company resources
allocated to anoncvs work. That is a valid concern from a maintainer's
perspective.

>>>>> "Jifl" == Jonathan Larmour <jifl@eCosCentric.com> writes:

    >> No, you still don't understand. My original proposal NEVER breaks
    >> compatibility. Not now, because it involved zero changes to the API.
    >> Not in future, because there will always be a cyg_flash_init() with
    >> the exact same interface as it does now. It will become deprecated but
    >> it will still exist.

    Jifl> Deprecating an API function means requiring API users to
    Jifl> stop using it. Are you requiring them to change their
    Jifl> application, freshly written to this new API, or not?

No. The correct solution, as I have said before is:

a) in flash.h, mark the function as deprecated;

    __externC int cyg_flash_init( cyg_flash_printf *pf ) __attribute__ ((deprecated));

  or even better, if gcc were to support it:

    __externC int cyg_flash_init( cyg_flash_printf *pf )
        __attribute__ ((deprecated ("explicit calls to cyg_flash_init() are no longer required")));

b) in flash.c, leave in place an implementation along the lines of:

    __externC int
    cyg_flash_init(cyg_flash_printf* pf)
    {
        cyg_flash_set_global_printf(pf);
	return 0;
    }

(cyg_flash_set_global_printf() and cyg_flash_set_printf() can be added
either now or at the point that cyg_flash_init() is marked deprecated.
There is little point in adding it now. The harm was that it involved
yet another change before the release branch could be cut, but that
has been superseded by events.)

Once cyg_flash_init() has been marked as deprecated, application
developers would get a new compiler warning. They can ignore the
warning and everything would continue to work. There is no
incompatibility.

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. If they are using strange hardware they may realize
that having the flash initialized earlier could cause problems, and
that it would be a good idea to move some init code from the
application to the platform HAL.

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.

(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. 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.)


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.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
Besuchen Sie uns vom 3.-5.03.09 auf der Embedded World 2009, Stand 11-300
Visit us at Embedded World 2009, Nürnberg, Germany, 3-5 Mar, Stand 11-300

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Flash subsystem update
  2009-02-20 12:55                 ` Bart Veer
@ 2009-02-20 21:22                   ` Jonathan Larmour
  0 siblings, 0 replies; 19+ messages in thread
From: Jonathan Larmour @ 2009-02-20 21:22 UTC (permalink / raw)
  To: Bart Veer; +Cc: john, ecos-maintainers

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2009-02-20 21:22 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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).