[ continuing the discussion on CDL issues from bug #1001550 ]
Jonathan Larmour wrote: > John Dallaway wrote: > > > The "set_value" keyword in ecos.db was introduced as a quick hack for use > > within the Red Hat test farm and was never intended to be used elsewhere. > > set_value will provide a user_value for the specified CDL item which can > > therefore be inadvertently changed using the "restore defaults" action in > > configtool. I would really like to consider the use of "set_value" to be > > deprecated. It should always be possible to use a separate tiny CDL-only > > package to achieve the same effect. Are you OK with this? > > Not really, no. Firstly, other targets use it. Secondly, the design intention > for CDL is that targets should be defined by platform packages, albeit with > "requires" rather than "set_value". As such this is much closer to the way > things are intended to be. Yes it originated as a solution to a specific > problem, but then so is ecos.db, which shouldn't exist at all. What we can do > at the moment is make the transition to a future improved world easier, so that > makes this approach better. Jifl, there are just four other instances of the use of set_value in ecos.db at present and in every case the command is actually unnecessary as it sets the user_value of a CDL option to the same value as the default_value. Discouraging the use of what is known to be a problematic command seems entirely reasonable to me. I think you may be underestimating how useful the "Restore Defaults" command is to regular configtool users. Certainly I would not regard this command 'obscure' by any means. Given that the design intention is to use platform packages to define targets, I don't understand why you would regard using a "set_value" command (located outside the HAL package hierarchy) as being closer to how things are supposed to be. I am suggesting that we use additional platform packages to set configuration options appropriate for specific targets. This seems absolutely aligned with the design intention which allows a combination of HAL packages to describe the hardware. There is a big difference between the use of "set_value" (an optional command) and the use of ecos.db itself which, for better or for worse, is an essential part of our infrastructure at present. If you remain unconvinced about deprecating "set_value", I suggest we talk and then summarise to the list. Ilija Kocho wrote: > Why isn't this available in CDL? I always wanted some kind of soft "requires", > that will set/change default value but not hinder user override. My proposed > name was supposed to be "recommends" but "set_value" is perhaps better. Jonathan Larmour wrote: > It isn't available in CDL because, as Bart would tell you, he's never been > given time to develop it. Although Bart did take a sabbatical year and > host-side CDL and configuration improvements was something he was going to be > working on in that time, I think in practice he found it difficult to make good > progress on it. I'm afraid I don't know what the status is or what the plans > are at the moment - it may be nearly complete, or he may not be developing it > much at the moment. Bart, what improvements did you have in mind? I am sure that many in the eCos community will have an opinion on how CDL can be improved. Let's keep any discussion on improvements to infrastructure open to all on this list. John Dallaway eCos maintainer http://www.dallaway.org.uk/john |
On 05/04/12 20:13, John Dallaway wrote:
> Jonathan Larmour wrote: [Wanting to stop using set_value] >> Not really, no. Firstly, other targets use it. Secondly, the design intention >> for CDL is that targets should be defined by platform packages, albeit with >> "requires" rather than "set_value". As such this is much closer to the way >> things are intended to be. Yes it originated as a solution to a specific >> problem, but then so is ecos.db, which shouldn't exist at all. What we can do >> at the moment is make the transition to a future improved world easier, so that >> makes this approach better. > > Jifl, there are just four other instances of the use of set_value in > ecos.db at present and in every case the command is actually unnecessary > as it sets the user_value of a CDL option to the same value as the > default_value. Actually, you also need to equivalently consider the use "enable", which is pointlessly used by the snds, but usefully used by the adder / adderII. > Discouraging the use of what is known to be a problematic > command seems entirely reasonable to me. I think you may be > underestimating how useful the "Restore Defaults" command is to regular > configtool users. Certainly I would not regard this command 'obscure' by > any means. Okay, hands up any configtool users (not maintainers) out there who have used this command, AND it's done what they wanted. Also, given that the config tool has a long-standing problem of invoking the inference engine incorrectly, I would consider "restore defaults" to be a rather risky operation - the graphical config tool's inference engine has the potential to do something different once things set via templates are unwound. It would certainly be extremely noisy. I would have thought no-one would use "restore defaults", but instead just create a new configuration. That's more straightforward, more familiar and more likely to work. "Restore defaults" is probably a misnomer, because it isn't the defaults you get when you create a new configuration (although if you are lucky it may end up that way after inference / conflict resolution). > Given that the design intention is to use platform packages to define > targets, I don't understand why you would regard using a "set_value" > command (located outside the HAL package hierarchy) as being closer to > how things are supposed to be. Because the setting of options is associated with the target entry, which comes from the platform HAL package. > I am suggesting that we use additional > platform packages to set configuration options appropriate for specific > targets. This seems absolutely aligned with the design intention which > allows a combination of HAL packages to describe the hardware. I'm not saying it wouldn't work. But I'm against a pointless package cluttering up the target-side repository. It's not like this may result in some hidden subtle effect. If you configure for the wrong hardware the effect will be obvious and instantaneous - it won't work. > There is > a big difference between the use of "set_value" (an optional command) > and the use of ecos.db itself which, for better or for worse, is an > essential part of our infrastructure at present. > > If you remain unconvinced about deprecating "set_value", I suggest we > talk and then summarise to the list. No, keep it on the list for anyone interested, including other maintainers, to be involved; like as you say further down in your section to Bart. Incidentally, you have the wrong email address for Bart, he is no longer at eCosCentric. I will forward your mail just in case. Jifl -- --["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine |
On 05/04/12 21:25, Jonathan Larmour wrote:
> On 05/04/12 20:13, John Dallaway wrote: >> Given that the design intention is to use platform packages to define >> targets, I don't understand why you would regard using a "set_value" >> command (located outside the HAL package hierarchy) as being closer to >> how things are supposed to be. > > Because the setting of options is associated with the target entry, which > comes from the platform HAL package. To be clear, I mean which _will_ come from the platform HAL package. One thing the design did not include was extra little HAL packages whose only raison d'ĂȘtre is to set a config option. Jifl --["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine |
In reply to this post by Jonathan Larmour-2
Hi Jifl
Jonathan Larmour wrote: > On 05/04/12 20:13, John Dallaway wrote: >> Jonathan Larmour wrote: > > [Wanting to stop using set_value] > >>> Not really, no. Firstly, other targets use it. Secondly, the design intention >>> for CDL is that targets should be defined by platform packages, albeit with >>> "requires" rather than "set_value". As such this is much closer to the way >>> things are intended to be. Yes it originated as a solution to a specific >>> problem, but then so is ecos.db, which shouldn't exist at all. What we can do >>> at the moment is make the transition to a future improved world easier, so that >>> makes this approach better. >> >> Jifl, there are just four other instances of the use of set_value in >> ecos.db at present and in every case the command is actually unnecessary >> as it sets the user_value of a CDL option to the same value as the >> default_value. > > Actually, you also need to equivalently consider the use "enable", which > is pointlessly used by the snds, but usefully used by the adder / adderII. I agree. So prior to your check-in we had 4 unnecessary instances of "set_value", 3 instances of "enable" (of which 1 was unnecessary) and no instances of "disable". >> Discouraging the use of what is known to be a problematic >> command seems entirely reasonable to me. I think you may be >> underestimating how useful the "Restore Defaults" command is to regular >> configtool users. Certainly I would not regard this command 'obscure' by >> any means. > > Okay, hands up any configtool users (not maintainers) out there who have > used this command, AND it's done what they wanted. No responses to date, but I think you would agree that this polling technique is not the most objective. :-) > Also, given that the config tool has a long-standing problem of invoking > the inference engine incorrectly, I would consider "restore defaults" to > be a rather risky operation - the graphical config tool's inference engine > has the potential to do something different once things set via templates > are unwound. It would certainly be extremely noisy. I would have thought > no-one would use "restore defaults", but instead just create a new > configuration. That's more straightforward, more familiar and more likely > to work. "Restore defaults" is probably a misnomer, because it isn't the > defaults you get when you create a new configuration (although if you are > lucky it may end up that way after inference / conflict resolution). In fact, "Restore Defaults" is much more useful when invoked at the CDL package or CDL component level rather than across the entire eCos configuration. Creating a new configuration is no substitute in these scenarios. >> Given that the design intention is to use platform packages to define >> targets, I don't understand why you would regard using a "set_value" >> command (located outside the HAL package hierarchy) as being closer to >> how things are supposed to be. > > Because the setting of options is associated with the target entry, which > comes from the platform HAL package. > >> I am suggesting that we use additional >> platform packages to set configuration options appropriate for specific >> targets. This seems absolutely aligned with the design intention which >> allows a combination of HAL packages to describe the hardware. > > I'm not saying it wouldn't work. But I'm against a pointless package > cluttering up the target-side repository. I don't think that using separate CDL packages to provide common and board-specific parts of a platform HAL is so very different from the partitioning of the HAL across multiple packages to accommodate processor variants, or other functional blocks with some common features. One of the great benefits of CDL is that it provides the flexibility to accommodate abstractions that were not necessarily envisaged at design-time. The capabilities are all there without the need for the set_value/enable/disable hacks that deliver user_values where default_values are required and were never intended to be used outside the Red Hat test farm. Jifl, it seems that both our views are quite firm. Since you have already checked-in the new code and it is unlikely to impact too many people, I am not inclined to argue this further. However, I would still recommend that eCos developers avoid use of the "set_value", "enable" and "disable" commands in ecos.db where possible. John Dallaway eCos maintainer http://www.dallaway.org.uk/john |
On 15/04/12 20:10, John Dallaway wrote:
> >>> Discouraging the use of what is known to be a problematic >>> command seems entirely reasonable to me. I think you may be >>> underestimating how useful the "Restore Defaults" command is to regular >>> configtool users. Certainly I would not regard this command 'obscure' by >>> any means. >> >> Okay, hands up any configtool users (not maintainers) out there who have >> used this command, AND it's done what they wanted. > > No responses to date, but I think you would agree that this polling > technique is not the most objective. :-) Well, it would only have taken one to put me in my place ;-). >> Also, given that the config tool has a long-standing problem of invoking >> the inference engine incorrectly, I would consider "restore defaults" to >> be a rather risky operation - the graphical config tool's inference engine >> has the potential to do something different once things set via templates >> are unwound. It would certainly be extremely noisy. I would have thought >> no-one would use "restore defaults", but instead just create a new >> configuration. That's more straightforward, more familiar and more likely >> to work. "Restore defaults" is probably a misnomer, because it isn't the >> defaults you get when you create a new configuration (although if you are >> lucky it may end up that way after inference / conflict resolution). > > In fact, "Restore Defaults" is much more useful when invoked at the CDL > package or CDL component level rather than across the entire eCos > configuration. Creating a new configuration is no substitute in these > scenarios. True, although that could cause even further unintended consequences when values in other packages had previously received inferred values from the package where "restore defaults" is applied. It's a subtly dangerous operation. >> I'm not saying it wouldn't work. But I'm against a pointless package >> cluttering up the target-side repository. > > I don't think that using separate CDL packages to provide common and > board-specific parts of a platform HAL is so very different from the > partitioning of the HAL across multiple packages to accommodate > processor variants, or other functional blocks with some common > features. It is a bit, given that people expect to find platform related options in the platform HAL, and now there would be a sort-of platform variant HAL stuck in the middle which is where the real meat is. It's something that I've never been very happy about with the AT91SAM7S HAL, although it's unavoidable there really as there is genuine board specific code there, albeit very little. That would not apply here - AFAICT the board is exactly the same (silk-screened "STM32 20-21-45-46 G-EVAL") but with a different CPU fitted. > One of the great benefits of CDL is that it provides the > flexibility to accommodate abstractions that were not necessarily > envisaged at design-time. While that's true, the concept of target templates being able to set values was something that /was/ envisaged at design time. > Jifl, it seems that both our views are quite firm. Since you have > already checked-in the new code and it is unlikely to impact too many > people, I am not inclined to argue this further. However, I would still > recommend that eCos developers avoid use of the "set_value", "enable" > and "disable" commands in ecos.db where possible. If anyone else wants to contribute to this (especially Bart of course) and say that your greater good is more important than my greater good, then fair enough, I'll go along with an outsider's view. Jifl -- --["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine |
Free forum by Nabble | Edit this page |