make attiny SPI code independent from different Arduino pins definitions

[troffmo5]

I wanted to use this library with PlatformIO for my attiny84 project.
It didn't work but i discovered that the cause was a different pin definition for the pinMode functions.
This patch uses the DDRx registry and makes the code independent of the different pin definitions.

[Avamander]

Someone with all the ATTiny's please confirm working in both Arduino IDE and PlatformIO.

[soligen2010]

You guys have a lot more experience than me on this library, but since you requested a review here are my comments.

Conceptually looks fine to me. I don't have an attiny to try it with but seems like is should work.

From a style/code organization perspective I would prefer all these #if statements to be in a .h file (like a pins.h or config.h) instead of the .cpp file. Have a #define for each pin with the statement to set it's mode then the .cpp can just use the pre-processor variable to run the right command. (also #defines to be used when reading/writing the pins would be good in the same place). I would also prefer to see each pin set independently on different lines of code instead of combining the two output pins on the same line.

These are my personal preferences on implementing something like this. I have had to change pin assignments in other projects before, and having something like a pins.h makes it a whole lot easier to find and make such changes.

[soligen2010]

I also noticed that the old code had a #define for 4 pins, but the new code only sets 3. Looks like there isn't anything to replace the #define SS. I don't know if this is an issue or not, maybe it was never used in the old code??

[troffmo5]

Hi,
i wanted to make the change small, just the scope of the fix. The #defines where in the previous implementation too. I agree that it is better to have functions/macros to set the pin direction and status. The problem is that the pinMode function relys on numbers defined outside the library therefore they can differ (e.g. 4 is not always mapped to PA6). By CSN and CE there is no problem because they are passed to the constructor. Building something like pinMode(PA6, OUTPUT) or digitalWrite(PA3, HIGH) would be a bigger change.

SS was apparently not used."SS" is normally used to indicate Slave Select, another name for CSN (passed to the constructor)

[plasticassius]

I use ATtiny84 in my projects, and while your change looks correct for this part (I didn't test it), it changes the way the pins are used in this one piece of code (to use the AVR register definitions instead of the Arduino macro pinMode), and leaves the Arduino macros (digitalWrite and pinMode) unchanged elsewhere (in the same source file). While the Arduino macros are less efficient than using the AVR definitions, the pinMode function is only called once at startup, so isn't an issue here.

The previous code showed both the Arduino pin numbers as well as the AVR definitions in the comments. Since you've removed the Arduino pin numbers which are used elsewhere, it makes the usage inconsistent (Arduino pin numbers in class RF24 and AVR definitions in class SPIClass).

I think readability is better the way it was before since it lists the names of the pins in both systems side by side. I would feel ok with your change if you didn't delete the defines even though they wouldn't be used since they serve to document the pins numbers in both numbering systems.

If the code wasn't working for you due to the Arduino pin numbers, perhaps you aren't compiling against the correct core. I use ATtiny84 and don't have this problem.

[TMRh20]

Tested ok