Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement SPI driver on ATmega2560 #4045

Conversation

danielamkaer
Copy link
Member

No description provided.

@thomaseichinger thomaseichinger added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Type: new feature The issue requests / The PR implemements a new feature for RIOT Platform: AVR Platform: This PR/issue effects AVR-based platforms labels Oct 5, 2015
@jnohlgard
Copy link
Member

@danielamkaer Please add a prefix to your commit message, like "cpu/atmega2560: Add SPI driver implementation", and also squash your whitespace fixes into the main commit.

I don't have the hardware available right now so I can't test. @N8Fear or @LudwigOrtmann, do you have any SPI devices that you can wire up to the Arduino?

#define SPI_1_EN 0

// SPI 0 Registers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use C comment style. // -> /* */

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch from ffc05f3 to 3664b33 Compare October 5, 2015 08:41
#if SPI_0_EN
case SPI_0:
SPI_0_DDR |= (1<<SPI_0_MOSI)|(1<<SPI_0_SCK);
SPI_0_DDR &= ~((1<<SPI_0_MISO));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines could be moved to spi_conf_pins?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The configuration of the pins depends on whether it is master/slave, no parameter for spi_conf_pins to configure that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, didn't see SPI_0_SCK changed lines.

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch from 3664b33 to 5678835 Compare October 5, 2015 08:44
switch (dev) {
#if SPI_0_EN
case SPI_0:
SPI_0_DR = out;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a ATmega expert, but wouldn't it be better to check if a previously set data register was already written out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can read from the data sheet, there isn't a way to check if the outgoing data register is empty. However the function will not return until SPIF is set, which indicates that the operation is finished.

@OlegHahm
Copy link
Member

OlegHahm commented Oct 5, 2015

Welcome to RIOT, @danielamkaer!

@thomaseichinger
Copy link
Member

@danielamkaer Great job! Please see some minor remarks above.

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch 2 times, most recently from be465fe to 297bf0c Compare October 5, 2015 09:10
@LudwigKnuepfer
Copy link
Member

I have, but I probably won't get around to testing this PR in the immediate future.

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch from 36a575d to 4e77d79 Compare October 12, 2015 06:43
@PeterKietzmann
Copy link
Member

Now that I gave up my Atmega2560 board, testing this PR might also be a job for @ReneHerthel :-)

@LudwigKnuepfer
Copy link
Member

With

diff --git a/boards/arduino-mega2560/Makefile.features b/boards/arduino-mega2560/Makefile.features
index 6d25415..ad1a466 100644
--- a/boards/arduino-mega2560/Makefile.features
+++ b/boards/arduino-mega2560/Makefile.features
@@ -1,4 +1,5 @@
 FEATURES_PROVIDED += periph_uart
 FEATURES_PROVIDED += periph_gpio
 FEATURES_PROVIDED += periph_timer
+FEATURES_PROVIDED += periph_spi
 FEATURES_MCU_GROUP = avr8

I get

Building application "periph_spi" for "arduino-mega2560" with MCU "atmega2560".

"make" -C /home/lo/RIOT/boards/arduino-mega2560
"make" -C /home/lo/RIOT/core
"make" -C /home/lo/RIOT/cpu/atmega2560
"make" -C /home/lo/RIOT/cpu/atmega2560/periph
"make" -C /home/lo/RIOT/cpu/atmega_common
"make" -C /home/lo/RIOT/drivers
"make" -C /home/lo/RIOT/sys
"make" -C /home/lo/RIOT/sys/auto_init
"make" -C /home/lo/RIOT/sys/shell
"make" -C /home/lo/RIOT/sys/shell/commands
"make" -C /home/lo/RIOT/sys/tsrb
"make" -C /home/lo/RIOT/sys/uart_stdio
/home/lo/RIOT/tests/periph_spi/bin/arduino-mega2560/periph_spi.a(main.o): In function `slave_on_cs':
/home/lo/RIOT/tests/periph_spi/main.c:135: undefined reference to `spi_transmission_begin'
collect2: error: ld returned 1 exit status
/home/lo/RIOT/tests/periph_spi/../../Makefile.include:213: recipe for target 'all' failed
make: *** [all] Error 1

Please ping me when this is resolved.

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch 2 times, most recently from 33013c6 to 9cf1db7 Compare October 26, 2015 07:54
@danielamkaer
Copy link
Member Author

@LudwigKnuepfer try now.

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch from 9cf1db7 to 8ed35ff Compare October 26, 2015 07:57
@LudwigKnuepfer
Copy link
Member

@danielamkaer: On a general note regarding our development procedures - please don't instantly squash your commits, it makes it easier for reviewers to see what changed. The "needs squashing" label is not there to alert you, but to prevent merging of unsquashed PRs.

@danielamkaer
Copy link
Member Author

Okay @LudwigKnuepfer - I'll remember for the future. This PR + 2 others are my first ever contributions to open source projects.

@LudwigKnuepfer
Copy link
Member

@danielamkaer No sweat, you'll (hopefully) get more feedback like this during your first steps. And remember: although some work has gone into our processes and policies, they are not perfect and can benefit from your input. So don't refrain from speaking out if you see room for improvement.

@LudwigKnuepfer
Copy link
Member

Can't test, don't know how to map GPIO pins to board layout anymore due to changes in RIOT =(
@haukepetersen I suspect you had your hands in this. Please tutor me ;) (Or even better: write some documentation...)

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch 2 times, most recently from 6e78880 to f24c0b4 Compare October 28, 2015 08:28
@danielamkaer
Copy link
Member Author

@LudwigKnuepfer - Lookup using https://www.arduino.cc/en/Hacking/PinMapping2560

Translate pin 4 (PG5) on Arduino into GPIO_PIN(PORT_G, 5)

@danielamkaer
Copy link
Member Author

Any progress?

@haukepetersen
Copy link
Contributor

@LudwigKnuepfer I guess we can shed some light on the pin mapping: we don't have a sub-set statically mapped anymore, but you can access any MCU pin freely using the GPIO_PIN(port, pin) macro. As @danielamkaer got it right, if you e.g. want to set pin PG.05 you just call gpio_set(GPIO_PIN(PORT_G, 5) or gpio_set(GPIO(5, 5)). The port enum and the GPIO_PIN macros are defined in cpu/atmega2560/cpu_periph.h.

Now mapping the acutal MCU pins to Arduino pins is another issue, just refer to the Arduino doc (e.g. the link posted above) for details. But I have an idea how to make this easier - will prepare a PR...

@haukepetersen
Copy link
Contributor

how about this: #4313. Now you can just use gpio_xx(ARDUINO_PIN_X)

@LudwigKnuepfer
Copy link
Member

Sorry, I did not have time for testing.. I would not feel hurt if anyone else tests this ;)

@OlegHahm OlegHahm modified the milestone: Release 2016.03 Dec 8, 2015
@zhuoshuguo
Copy link
Contributor

@danielamkaer Ping. A kind reminder for rebase.

@danielamkaer
Copy link
Member Author

@zhuoshuguo I need some help I think. I'm trying to do
$ git pull upstream master --rebase
but I run into a lot of conflicts, which are not caused by my changes.

@LudwigKnuepfer
Copy link
Member

@danielamkaer
I just did a local git rebase upstream/master with your branch and that went fine.
Maybe you want to git reset --hard origin/create_spi_driver_for_atmega2560 in order to reset your branch (throwing away all new changes in case there are any, so be careful) before.

@danielamkaer danielamkaer force-pushed the create_spi_driver_for_atmega2560 branch from f24c0b4 to ddde457 Compare February 25, 2016 08:43
@OlegHahm
Copy link
Member

@thomaseichinger, @haukepetersen, @PeterKietzmann, anyone for a review?

@PeterKietzmann
Copy link
Member

Yes, in general but not so much time currently. At the latest during the next Hack'n'ACK

@PeterKietzmann PeterKietzmann added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Feb 29, 2016
@kYc0o
Copy link
Contributor

kYc0o commented Feb 29, 2016

Maybe @aabadie would like to try this? (:

@aabadie
Copy link
Contributor

aabadie commented Feb 29, 2016

Thanks @kYc0o. Why not, if I have time.

@haukepetersen
Copy link
Contributor

Hi, just tested this PR, for me it does not work. Connecting a logic analyzer to pins 50-52 does not show me any pin activity when sending data on the bus...

@haukepetersen
Copy link
Contributor

While starting to debug the driver, I did so many changes, that I decided to make a new PR -> #5020.

@danielamkaer: Sorry again for starting to actually test this so late! It's the good old thing that you only start with something until you really need it...

@haukepetersen
Copy link
Contributor

closed in favor of #5020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Platform: AVR Platform: This PR/issue effects AVR-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet