RTEMS_DECONST - Should it be removed? - TMS570

Wed Jan 21 20:35:01 UTC 2015

Hello Martin,

On Wednesday 21 of January 2015 19:44:21 Martin Galvan wrote:
> Sorry for the late answer.

no problem. By the way, I would be off from Friday whole next week.

> On Wed, Jan 14, 2015 at 6:50 PM, Pavel Pisa <pisa at cmp.felk.cvut.cz> wrote:
> > Hello Martin,
> >
> > On Wednesday 14 of January 2015 18:27:41 Martin Galvan wrote:
> >> Hi everyone! We're currently working on improving the TMS570 BSP, and
> >> in the process we discovered an important bug caused by a misuse of
> >> the RTEMS_DECONST macro. Said macro seems to be used in a few other
> >> places throughout the code to bypass const restrictions.
> >
> > Please, can you provide more details?
> > I have contributed to RTEMS_DECONST and TMS570 BSP - directly
> > and as GSoC menthor. So bugs can fall to my head.
>
> Sure. We're working with a TMS570 USB Development kit, with both RTEMS and
> the HALCogen-generated code placed the board's ROM (I understand you were
> loading RTEMS in RAM).

I have tested both but most testing has been done with code running
from internal and from external RAM to lower Flash wearing.

> What happened was that, in the SCI driver, driver_context_table was
> declared as const and it included a variable called tx_chars_in_hw which is
> used by the interrupt handler as a flag to mark that we just wrote a
> character. tx_chars_in_hw was supposed to be set to 1 in
> tms570_sci_interrupt_write; however because driver_context_table was
> declared as const it was being placed inside the board's ROM. Therefore,
> tx_chars_in_hw couldn't be written into, and the console got locked after
> writing the first character. RTEMS_DECONST was being misused in
> console_initialize to cast &driver_context_table.base to
> rtems_termios_device_context *, thus masking out the source of the bug. The
> only reason it was working for you is that RTEMS was loaded in RAM.

That is important catch. If the structure/descriptor is const then
work variable has to be moved to some other place/structure for sure.
Bad that we have not catch that. So we need to check if driver_context_table
should be read/write or if there should be other structure for runtime
modified data.

Generally tx_chars_in_hw is required due way how generic RTEMS serial
support is done. You change it to bool which is OK when there is no
FIFO in HW. But if there is FIFO enabled then count has to be kept.
I have feelt some parameters and return values selection in generic driver
base as little unfortunate in this regard.
I would see as more natural if TX HW fill function returns number
of characters which it has been able to push to HW and this information
is processed by generic serial driver code. No need for such variable exists
in the context case.
But I need to recheck that I am right and that mainline code has not been
changed.

You have removed parity and others termios options application from 
tms570_sci_set_attributes(). It should be implemented there generally,
have you some hints what is broken in original code? Premysl Houdek
should to correct that in longer therm.

> Right now we're facing a different bug where we can't use printf to print a
> 2-digit integer because the system hangs somewhere inside printf. This
> happens in both interrupt-driven and polled mode, but it doesn't happen
> with puts or printk (which leads us to think it's either a printf or a
> driver issue). Any pointers on this would be welcome.

Strange, because we have run many tests, but it can be similar
catch/omission when code runs from Flash.

> >> What's the purpose of having something like this in the codebase?
> >> Should it be removed to maintain const-correctness throughout the
> >> code?
> >
> > The RTEMS_DECONST is the tool how to suppress warning when
> > the target function can cope with const and non-constant
> > pointer, can distinguish if it is non-const case from some
> > data in from the poited to structure or other parameter
> > and it needs to change some data/reference counts in non-const
> > case for example. Such cases cannot be solved cleanly with
> > C in some specific cases and programmer has to take  responsibility
> > into his/her own hands. But there could be bug for sure.
>
> I noticed that. It's unfortunate we have to have something like this in the
> codebase; we should at least add a few comments to the macro definition to
> avoid having people using it unless it's completely necessary.

It is bad if it it used somewhere only to suppress warning without
deeper analysis. But C and sometimes even C++ does not provide
solution which is clean and optimal.

> > As for TMS570 BSP, Premysl Houdek is due to finish some
> > things in the BSP in the next months. He needs to complete
> > his master thesis and submit it.
> >
> > I have spent considerable time with him to negotiate
> > and prepare format and way to extend coverage of TMS570 header
> > files. I have communicated with Ti representatives about
> > releasing files under RTEMS compatible license but they are
> > not able to change that for more reasons. So we work on preparation
> > of RTEMS license and format friendly header files from PDF manual.
>
> I don't understand what you meant by that. Right now we're using the
> HALCogen code for testing purposes only; is there any way to incorporate
> them into the BSP code?

You can use HalCoGen files in your proprietary or open source application
but resulting code combination is not GPL license compatible
(or at least Ti does not approve/know that). This means that no
code or headers fragment (which is larger than some minimal size
which makes code to fall under copyright) can be included
in RTEMS mainline.

> > Premek has promised to complete ETHERNET support in frame
> > of his thesis. My other colleague finished work on TMS570 ETHERNET
> > for Ti provided FreeRTOS to really work. So we have knowledge
> > now how the EMAC works. We work on Matlab/Simulink support
> > in our other industrial partner paid non-RTEMS TMS570 and RM48
> > project based on Ti provided CodeComposer support. So I would be happy
> > if we can one day extend TMS570 to support RM48 as well but that
> > is far far future without contract. But I hope that TMS570
> > will be extended in next months.
>
> That's great. We're not planning to use ethernet with the TMS right now so
> we won't be able to test it, but any improvements to the BSP are welcome.
>
> > So please, if you plan to introduce more changes, please, send
> > plan of your work in advance to coordinate things.
>
> We're probably gonna have to refactor a few of the existing files and add
> CAN support.

We have complete CAN and FlexRay support for Ti FreeRTOS and HalCoGen
based codebase. But problem is again with copyright even that most
of the code has been developed by us. So if you start clean room
implementation we can help somewhere. But even if we start with RTEMS
we need to reimplement most of that. That is why we wait for Premysl
Houdek's preparation of header files from PDF to and then check
what from our sources can be ported to new headers. Anyway, CAN is
above size of his diploma thesis plans. You can look for API of our
CAN and FlexRay library in the document

  http://rtime.felk.cvut.cz/rpp-tms570/rpp_simulink.pdf

There has been attempt to follow Autosar requirements when these APIs
has been develop.
If the RTEMS API is similar to above mentioned one then porting
of TMS570 Matlab/Simulink support to RTEMS would be easier.

You can look at our port of TMS570 CAN/Simulink code to Linux SocketCAN there.

  http://rtime.felk.cvut.cz/gitweb/socketcan-simulink.git

This layer is completely independent of HalCoGen and is BSD like
licensed from us so it is fully compatible with RTEMS. But middle layer
is more complicated problem.

> > If you want to text TMS570 for real application which is computational
>
> intensive
>
> > then you probably want to try change board configuration files to specify
> > hat FPU should be used for math. We have used soft-float
> > for initial porting but thanks to Sebastian Huber generic
> > ARM work, TMS570 float is supported too.
>
> Right now we're not using the FPU, but we'll definitely need it in the near
> future so that's great.
>
> > As for RTEMS_DECONST, its definition should not be removed for sure.
> > But it is possible that it is used in incorrect place or even that
> > that macro has bug. So please, specify in more detail what is
> > the problem and what are your planes.
>
> I'm attaching a patch with the refactored driver. It should work with the
> tms570ls3137_hdk configuration. Notice we had to add some calls to
> HALCogen-generated functions inside the RTEMS initialization code in order
> for everything to work. We also changed the value of the BSP_PLL_OUT_CLOCK
> macro from 160 MHz to 180 MHz.

I do not get to this this and next week. But we definitely look how
to correct the problem. As for the HalCoGen dependency, we strive to not
introduce it. That is why we used separate HalCoGen loader for now and
longer time plan is to include needed initialization into RTEMS BSP
and as an alternative, disable low level setup in RTEMS and use some
open source loader - it would be interesting to have U-boot port there.
I know how to write Flash updater but again Ti provided library is binary
only and not license compatible to be included in RTEMS sources.

On Wednesday 21 of January 2015 19:59:09 Martin Galvan wrote:
> One more thing: you may notice we have two forms of the prescaler
> formula. The one that's commented out is the (incorrect) one that
> HALCogen generates; using that one gives us a prescaler of 48 while
> using the (supposedly correct) one yields 47. The bug we're currently
> facing seems to happen more often if we use 47 for the prescaler;
> that's why I left it uncommented.

We strive to write code for RTEMS without use of HalCoGen code.
We sometimes analyze, compare with manual and use generated values

You can find our SDRAM analysis below for example

https://rtime.felk.cvut.cz/hw/index.php/TMS570LS3137#SDRAM_setup

Best wishes,

             Pavel