[Bug 1001550] STM32 F2 and STM3220G-EVAL / STM3240G-EVAL contribution from eCosCentric

[bugzilla-daemon at bugs dot ecos dot sourceware dot org]

Please do not reply to this email. Use the web interface provided at:
http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001550

--- Comment #12 from Jonathan Larmour <jifl@ecoscentric.com> 2012-04-02 16:08:00 BST ---
(In reply to comment #9)
> Jifl, great to see this contribution!
> 
> Some initial comments/queries based on a quick scan of the patches:
> 
> * An earlier patch to the STM32 serial driver for bug #1001068 appears to have
> been reversed by your patch. Can you reinstate this please?

No it wasn't reversed. It was an alternative implementation to fix the same
problem. The logic is identical. But it's true that checking it in was
unnecessary, so I will revert the change anyway, to avoid pointless unnecessary
code changes. Plus the previous version was slightly clearer (evidently given
you misread it!).

> * Some of the changes listed in the STM3210E-EVAL platform HAL ChangeLog do not
> appear to be present in the source patch (eg the .ldi file changes). Can you
> check that the various ChangeLogs provide an accurate reflection of the changes
> committed please? Ref:
> 
> http://bugs.ecos.sourceware.org/attachment.cgi?id=1684&action=diff

It's the other way round - that was a ChangeLog entry that was missed earlier.
As you know, the STM3210e port was developed in eCosCentric, and the ChangeLog
should reflect the train of development (even if superseded by subsequent
events), and I noticed this ChangeLog entry was missing. It would have been
rather pointless to commit it separately.

> * The ecos.db patch lists the STM32 ethernet and watchdog packages for the new
> targets and some relevant register definitions are provided at the HAL level
> but I don't see the packages in the CVS commit. Ref:
> 
> http://bugs.ecos.sourceware.org/attachment.cgi?id=1683&action=diff#ecos.db_sec2

The ecos.db that was committed is correct. I had changed it following my
testing but evidently forgot to regenerate the patch, that's all. You can just
ignore those lines.

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

I would also be quite against a proliferation of pointless tiny packages across
the repository, cluttering things up. If that has already happened anywhere,
then that's a shame. Specifically, I think these considerations override a
particular facet of behaviour of an obscure config tool property. I don't
really care enough since it's obscure, but it seems more likely to me that it
is the config tool which needs a fix in that case. We should not have to change
target-side stuff to accommodate peculiarities of the config tool.

> Finally, are you able to confirm that the code you have checked in has been
> tested (eCos and RedBoot run-time) in the context of the eCos CVS HEAD? This is
> a substantial patch and I'm just concerned that we don't destabilise the
> existing STM32 support.

The only existing STM32 support is the STM3210e, which also came from
eCosCentric. But yes both eCos and RedBoot have been tested, albeit not to the
level of eCosCentric's own ports. But since the point is that the code is now
more similar to eCosCentric's sources, you can expect it to be high quality.

I will also update the website now.

Jifl

-- 
Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.