code problem with library

I’ve been trying to adapt the lowpower labs RFM12 library for the ATmega1284. It’s proving to be quite a challenge. I ran across a source code issue that makes no sense to me, and was wondering if someone might take a look see. Follows are lines 49…105 in the source here

RFM12B::Byte() is a function that simply writes a byte to the SPI port. The question at issue is below.

uint8_t RFM12B::Byte(uint8_t out) {
#ifdef SPDR
  SPDR = out;
  // this loop spins 4 usec with a 2 MHz SPI clock
  while (!(SPSR & _BV(SPIF)));
  return SPDR;
#else
  // ATtiny
  USIDR = out;
  byte v1 = bit(USIWM0) | bit(USITC);
  byte v2 = bit(USIWM0) | bit(USITC) | bit(USICLK);
#if F_CPU <= 5000000
  // only unroll if resulting clock stays under 2.5 MHz
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
  USICR = v1; USICR = v2;
#else
  for (uint8_t i = 0; i < 8; ++i) {
    USICR = v1;
    USICR = v2;
  }
#endif
  return USIDR;
#endif
}

uint16_t RFM12B::XFERSlow(uint16_t cmd) {
  // slow down to under 2.5 MHz
#if F_CPU > 10000000
  bitSet(SPCR, SPR0);
#endif
  bitClear(SS_PORT, cs_pin);
  uint16_t reply = Byte(cmd >> 8) << 8;
  reply |= Byte(cmd);
  bitSet(SS_PORT, cs_pin);
#if F_CPU > 10000000
  bitClear(SPCR, SPR0);
#endif
  return reply;
}

void RFM12B::XFER(uint16_t cmd) {
#if OPTIMIZE_SPI
  // writing can take place at full speed, even 8 MHz works
  bitClear(SS_PORT, cs_pin);
  Byte(cmd >> 8) << 8;
  Byte(cmd);
  bitSet(SS_PORT, cs_pin);
#else
  XFERSlow(cmd);
#endif
}

Do the following 2 lines (99+100) in RFM12B::XFER(uint16_t cmd) make any sense, or should they look like the similar lines (87+88) in RFM12B::XFERSlow(uint16_t cmd)? I can’t see what the first one accomplishes.

  Byte(cmd >> 8) << 8;
  Byte(cmd);

By default

#define OPTIMIZE_SPI       1  // uncomment this to write to the RFM12B @ 8 Mhz

The code does seem to work, since the RFM12 seems to communicate fine when using UNO and Mega2560 boards. Thanks for any guidance.

oric_dan:
Do the following 2 lines (99+100) in RFM12B::XFER(uint16_t cmd) make any sense, or should they look like the similar lines (87+88) in RFM12B::XFERSlow(uint16_t cmd)? I can’t see what the first one accomplishes.

  Byte(cmd >> 8) << 8;

Byte(cmd);

The “<< 8” at the end of the first line makes no sense and can be removed.

The remaining two calls to Byte() send the high byte and low byte of a 16-bit value.

Yeah, same thinking for me. I was surprised the 1st line even compiles without producing an error. Thanks John.

oric_dan:
Yeah, same thinking for me. I was surprised the 1st line even compiles without producing an error. Thanks John.

Why should it produce an error? It’s just running a function and throwing away the results. It’s no different to

void main() {
    1;
}

which compiles fine. Take the literal 1 and assign it to nothing. Just the same as calling a function that returns a value (like SPI.transfer()) and not assigning the return value to anything.

<< can be thought of as a function:

int value = <<(cmd, 8);

and in fact in C++ “operators” are functions - you can define them inside your classes, so when that operator is used on the class instance the function is called.

MyClass MyClass::operator+(const MyClass &rhs) {
  MyClass retval;
  retval.coreValue = this.coreValue + rhs.coreValue;
  return retval;
}

oric_dan:
I was surprised the 1st line even compiles without producing an error.

It’s a valid expression and therefore a valid statement. It just doesn’t make sense because the results of the << operation aren’t stored anywhere or used for anything.

Thanks, both of you. I guess it's a valid expression, and doesn't trip the compiler, but I suspect the author of the library just failed to clean up a little leftover mess. Only one of many little messes in that library.

So many of these darn 3rd party libraries are written to dictate rigid pin usage, and don't play so well with other libraries (eg, use of SPI ports, CS pins, interrupt pins, etc), :-(.

oric_dan:
Thanks, both of you. I guess it’s a valid expression, and doesn’t trip the compiler, but I suspect the author of the library just failed to clean up a little leftover mess. Only one of many little messes in that library.

So many of these darn 3rd party libraries are written to dictate rigid pin usage, and don’t play so well with other libraries (eg, use of SPI ports, CS pins, interrupt pins, etc), :-(.

I’m always a big proponent of abstracting things like SPI usage - the SPI library should be allocating “devices” to things that want to use SPI - the device contains all the information about how to do the SPI transfer - speed, spi mode, CS pin, etc. Then when you do any form of SPI transfer the SPI library automatically ensures that the SPI module is configured in the right way for that transfer, and deals with controlling the CS pin for you. That way many different SPI devices can play together nicely without any one causing another to fail because it changed the SPI mode or something.

Yes, it takes a bit of foresight and pre-planning, doesn't it. You'd think that providing an init() function that allows selecting CS pin and INT number for the device would be an obvious inclusion. Oh well.