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