Closed GoogleCodeExporter closed 8 years ago
I don't think this is the right solution to the problem. I accept there are
cases where a platform-specific implementation of HdlcUart is valuable. As
I understand the basis for this change, it's because tos/lib/ppp is not in
the .platform @includes list for any platform, so the use of PFLAGS +=
-I$(TOSDIR)/tos/lib/ppp to provide access to it causes that implementation
to supersede any platform-specific implementation. By moving it to
tos/lib/serial, which is in all .platform @include lists, it can be
overridden in a platform-specific way.
On reviewing all this, I've come to a couple conclusions:
* Though I named PlatformSerialHdlcUartC that way because it was an
implementation of HdlcUartC that was based on PlatformSerialC, it was not
intended to itself be a Platform*C component. If moved to tos/lib/serial
as proposed, it would be the first example of a so-named component that
had both shared and platform-optimized implementations. In fact, it
should be renamed to something like DefaultHdlcUartC.
* All the component does is implement HdlcUart, an interface that is not
being proposed for use anywhere except in tos/lib/ppp, where its main
value is in feeding into HdlcFraming, which is still pretty specific to
PPP. I don't think a component in tos/lib/serial (available on all
platforms) should have a dependency on an interface in tos/lib/ppp (must
be explicitly enabled).
* PlatformSerialHdlcUartC is not used in any components that are not
applications, so explicitly selecting an alternative is not hidden.
I propose the following alternative solution:
* I will commit a patch that renames the component and all its uses to
DefaultHdlcUart[CP]. (I want to do this because it's not quite that
simple: there are unit tests that must also be modified which, by the way,
fail because they specifically test that implementation, and thus fail in
the branch that uses the telosa variant.)
* You (sdhags) commit your updated PlatformHdlcUartC [sic] component inside
the telosa directory, then use code like:
#if PLATFORM_TELOSA /* or other platforms providing this */
components PlatformHdlcUartC;
PppDaemonC.HdlcUart -> PlatformSerialHdlcUartC;
PppDaemonC.UartControl -> PlatformSerialHdlcUartC;
#else
components DefaultHdlcUartC;
PppDaemonC.HdlcUart -> DefaultHdlcUartC;
PppDaemonC.UartControl -> DefaultHdlcUartC;
#endif
in PppRouter and any other application which needs the sped-up version.
What do you think, sirs?
Original comment by pabi...@gmail.com
on 13 Jun 2011 at 12:46
I think you are right since we aren't and don't want to propose a new
platform-wide abstraction for this purpose. Since we can't do this system-wide
(by the implementor of UartStream) without breaking the semantics of that
interface, this seems reasonable and consistent what has been done in other
places. I did notice that the test cases break because they pull in
SerialPrintfC and thus the other uart stack; what I did was just remove the
printf dependency but clearly this can't actually get checked in; it would be
nice if I could run the tests using the DMA driver to check that it works the
same way, though.
I'll keep an eye out for those changes and make the changes you suggest once
they show up.
Original comment by sdh...@gmail.com
on 13 Jun 2011 at 7:04
Rename completed in r5645 on trunk.
Original comment by pabi...@gmail.com
on 18 Jun 2011 at 8:43
Original issue reported on code.google.com by
sdh...@gmail.com
on 10 Jun 2011 at 12:50