[Tinyos Core WG] AdcConfigure and struct return

[Vlado Handziski]

On 9/21/06, David Gay <dgay42 at gmail.com> wrote:
>
> On 9/21/06, Vlado Handziski <vlado.handziski at gmail.com> wrote:
> > On 9/20/06, David Gay <dgay42 at gmail.com> wrote:
> > > [Results at end. Summary: runtime cost is 5x slower for Read.read, a
> > > 370 byte code increase and some RAM cost]
> >
> > This is strange. See below...
>
> You returned pointers to structures, not the structures themselves. I
> used structures rather than pointers to const structures, because
> that's more comparable to what you get from separate commands - see
> discussion at end. In theory, a compiler could do just as good a job
> with all three approaches; in practice the order is "commands - const
> structures pointers (similar) - structures (a lot worse)".


We talked on the telecon that we are changing the code to pass pointers
instead of the structs.

Btw, your code is buggy, because you returned a pointer to a local
> variable - I hope you're using static on the msp430 for your
> equivalent code (no, there's no realistic guarantee that some future
> version of gcc won't break it w/o the static).


Are you referring to the module variables? The code has nothing to do with
Jan's implementation on the msp430. I did a quick test only to double check
the overhead that you reported because it looked incredibly large. But on
msp430 a const goes into flash automatically, so I don't see a problem.

> > the code that uses this from
> > >   uint8_t channel() {
> > >     return call Atm128AdcConfig.getChannel[client]();
> > >   }
> > >   uint8_t refVoltage() {
> > >     return call Atm128AdcConfig.getRefVoltage[client]();
> > >   }
> > >   uint8_t prescaler() {
> > >     return call Atm128AdcConfig.getPrescaler[client]();
> > >   }
> > >   void sample() {
> > >     call Atm128AdcSingle.getData(channel(), refVoltage(), TRUE,
> > prescaler());
> > >   }
> > >
> > > to:
> > >   uint8_t channel() {
> > >     return (call
> > Atm128AdcConfig.getConfiguration[client]()).channel;
> > >   }
> > >   uint8_t refVoltage() {
> > >     return (call
> > Atm128AdcConfig.getConfiguration[client]()).refVoltage;
> > >   }
> > >   uint8_t prescaler() {
> > >     return (call
> > Atm128AdcConfig.getConfiguration[client]()).prescaler;
> > >   }
> > >   void sample() {
> > >     call Atm128AdcSingle.getData(channel(), refVoltage(), TRUE,
> > prescaler());
> > >   }
> >
> > Why do you insist on keeping the 3 calls?
>
> Why not (I found the code more readable that way) ? With inlining, one
> would hope the optimiser wouldn't care (and in fact it doesn't - those
> small functions are inlined.


I think that gcc is not smart enough in handling those "inlined function
returning sturct".something

Anyway, with the structures in flash, and returning a pointer to
> flash, the code overhead is 60 bytes and the cycle overhead is 30,
> which isn't too bad.


Cool. Now we agree that it is not an issue of performance anymore, but of
religion :)

But: I see very little point in doing this. We now have an interface
> which has a single command which returns an opaque value (except for
> platform-dependent code). This is no more portable or
> hardware-independent than an interface with opaque commands.


I agree that it is not completely platform-independent. But it does have
provide a platform independent interface. The same way as dev_read or
dev_ioctl, the command is independent, but the payload/parameters are not.

Additionally, it's not really more flexible: you trade off the ability
> to pass around to play with these opaque values in C code (I suppose
> you could pick different configurations based on some runtime option,
> though you could still do something very similar with parameterised
> interfaces). You lose the option of getting configuration values in
> more interesting ways (all those configuration structures have to be
> compile-time constants if you want reasonable code).


One interesting application in keeping the configs into a struct is a
getSequence  command. On the msp430 you can specify a sequence of channels
to be sampled in a single go (<16). One natural way to express this in an
API is for the user to pass an array of pointers to the configuration
structs that are in flash.

For instance, the current photo sensor's channel is defined as:
>   async command uint8_t Atm128AdcConfig.getChannel() {
>     return call PhotoAdc.getChannel();
>   }
> where PhotoAdc is connected to a per-platform configuration component
> which specified mapping of the mica bus adc channels to the particular
> platform's adc channels.
>
> To do this with configuration structures, we'd have to use constants
> instead (possible, of course).


As I said, it has to do with history (we have always used structs on the
msp430, from the very first implementation) and religion, i.e. our
understanding of what a HIL is. In the spirit of the original HAA paper, a
HIL should be realy platform independent. I tried to push this view many
times but no one wanted to embrace this. No one was willing to make the
additional step of implementing in software a missing HW functionality or to
agree on common parameters, etc. The main point of the HAA was that if you
don't want to pay this price, you should stick with the HAL. But if we keep
the original meaning of HIL, we will see that today in TinyOS 2.0 we have
only one abstraction that deserves the name, i.e. the timers, and in some
degree the flash also. All other "HILs" are nothing more than  "HAL2s".

This being such a major issue, I invite once again all interested parties to
participate in the HIL telecon next thursday. Hopefully we can come to some
agreement.

Vlado
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.millennium.berkeley.edu/pipermail/tinyos-2.0wg/attachments/20060921/98db6bd1/attachment.htm