CDL usage and improvements

[John Dallaway-2]

[ 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

[Jonathan Larmour-2]

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

[Jonathan Larmour-2]

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

[John Dallaway-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

[Jonathan Larmour-2]

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