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.

Sign up for GitHub

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

Jump to bottom

Arduino-mega2560 GPIO implementation #2799

Merged
merged 1 commit into from
Sep 7, 2015
Merged

Arduino-mega2560 GPIO implementation #2799

merged 1 commit into from
Sep 7, 2015

Conversation

ReneHerthel
Copy link
Contributor

-Implements some GPIO functionality for arduino-mega2560, with cpu ATmega2560, for pins 2 -13 at PWM

  • Add missing functions: gpio_init_out / init_in / read / set / clear / toggle / write.

@PeterKietzmann PeterKietzmann self-assigned this Apr 13, 2015
@PeterKietzmann PeterKietzmann added the Area: drivers Area: Device drivers label Apr 13, 2015
@PeterKietzmann
Copy link
Member

@ReneHerthel nice! Will review soon

PeterKietzmann
PeterKietzmann reviewed Apr 13, 2015
View reviewed changes
boards/arduino-mega2560/include/periph_conf.h
#define GPIO_14_EN 0
#define GPIO_15_EN 0
#define GPIO_NUMOF 12
#define GPIO_2_EN 1
Copy link
Member

Choose a reason for hiding this comment

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

Why didn't you implement GPIO_0 and GPIO_1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because there are pin 2-13 at PWM- and 0-1 in communication-section on the pinout.
for example: if you want to set pin 2, you have to call the method with 2 (parameter: dev).
But when you change the pin_map, that GPIO_0 is first, you will have the wrong pin all time, so i decided to implement it this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should i change it from GPIO_2 to GPIO_0... ?

Copy link
Member

Choose a reason for hiding this comment

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

The advantage of the periph_conf is that you can abstract the hardware-dependent stuff from your driver. That means the "GPIO_0" could for example either be port E pin 5 or port H pin 6... Please change this and all dependencies. You don't need to implement 15 GPIO pins (even if it would be nice :-) ). It is more important to prevent potential pin conflicts for later driver implementations. Tha means: Try to avoid implemention GPIOs when blocking other peripheral pins with that. Example: If there are just 3 pins that can be configured as SPI-pins, don't implement the GPIO functionality on them. Ok?

@PeterKietzmann
Copy link
Member

I don't know this board in detail. Where is the relation between PWM and GPIO? Isn't it possible to implement them independently?

PeterKietzmann
PeterKietzmann reviewed Apr 13, 2015
View reviewed changes
cpu/atmega2560/periph/gpio.c

int gpio_init_out(gpio_t dev, gpio_pp_t pushpull)
{
return -1;
uint8_t pin;
pin = gpio_pin_map[dev - 2];
Copy link
Member

Choose a reason for hiding this comment

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

I know, you started your implementation at GPIO_2 but I don't know the reason for this. What if one tries to initialize GPIO_0 (which has the value 0) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't noticed that. Thanks

@PeterKietzmann
Copy link
Member

The interrupt functionality is missing

@PeterKietzmann
Copy link
Member

@ReneHerthel nice work and congrats to your first RIOT-PR :-) !

PeterKietzmann
PeterKietzmann reviewed Apr 13, 2015
View reviewed changes
cpu/atmega2560/periph/gpio.c
uint8_t pin;
pin = gpio_pin_map[dev - 2];
switch(dev) {
case 2:
Copy link
Member

Choose a reason for hiding this comment

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

It would be much prettier using the GPIO names here. (So 2 will become GPIO_2 and so on)

@ReneHerthel
Copy link
Contributor Author

@PeterKietzmann Hmm, yes it is always low, when you apply 3.3V or GND.
It's only HIGH, when the bit is set in the register.
If you set the input to HIGH with set 0 0, the output after read 0 0 is HIGH.
If you clear the same pin the output is LOW.

But you mean, it should print HIGH, when you attach a wire to the pin with GND or 3.3V?
The gpio_read function is only looking at the bit, if it's set or not.

Or you think that the register changed automatically, when you apply something with an high value or GND?

And the E5 thing, no idea at the moment :-/

@PeterKietzmann
Copy link
Member

@ReneHerthel please adapt Haukes proposed changes. Regarding the read function: of course you want to get to logical value that corresponds to an external "logocal" signal. Why should you want to read a value that you set by yourself? Then the function is not implemented correctly. Regarding interrupts: Let's see if the same applies to INT6 and INT7. Maybe that gives exposure. If not we need to spend some time in this together (or so)..

@ReneHerthel
Copy link
Contributor Author

@PeterKietzmann
I think I got it, we used only PORTx and DDRx before, now we need the PINx register to read the input.

Btw. can you tell me which two pins are for INT6 and INT7? I found out it should be PE6 and PE7, but I don't know which physical pins they are on the board. Thats why I don't implement it before..

@PeterKietzmann
Copy link
Member

@ReneHerthel honestly I don't know either. But as this is a CPU dependant implementation we should not care. Imagine this CPU is used on an other board design. There, it may appear that all interrupt lines have a hardware pin. So the read function works now as expected? I'll test it next week and then the last thing should be the interrupt-bug.

@ReneHerthel
Copy link
Contributor Author

@PeterKietzmann Ah, okey.
I think E5 is already fixed.
I can initialize E5 as interrupt and connect it with another pin, so you can toggle the other pin to do some interrupts on E5.
Read should work, also on E5.
You can apply 3.3V to read HIGH or GNDto read LOW on an input pin.

Anyway, I'll come tomorrow, so we can finish this and maybe test it with an oscilloscope :-)

PeterKietzmann
PeterKietzmann reviewed Sep 7, 2015
View reviewed changes
cpu/atmega2560/periph/gpio.c
@@ -0,0 +1,255 @@
/*
* Copyright (C) 2015 haw-hamburg
Copy link
Member

Choose a reason for hiding this comment

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

Write it like "HAW Hamburg" please

@PeterKietzmann
Copy link
Member

@ReneHerthel, now everything works as expected and the code quality has really improved over the weeks. Please address my last minor comments and add a commit. I think we can merge it then :-)

@ReneHerthel ReneHerthel force-pushed the arduino_2560_gpio branch 2 times, most recently from f964a73 to d05151f Compare September 7, 2015 12:48
@PeterKietzmann PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Platform: AVR Platform: This PR/issue effects AVR-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Sep 7, 2015
@PeterKietzmann
Copy link
Member

Travis fails for two groups and I don't understand why. Could it be related to #3777?

$ source ./dist/tools/pr_check/check_labels.sh

$ test -z "$TRAVIS_PULL_REQUEST" || test "$BUILDTEST_MCU_GROUP" = "static-tests" || check_gh_label "Ready for CI build" || exit 1

@haukepetersen , @kaspar030, @phiros any ideas?

@kaspar030
Copy link
Contributor

@PeterKietzmann #3777 is unlikely to cause this.

@kaspar030
Copy link
Contributor

But travis seems to be very unresponsive right now... Can't even restart the build.

@PeterKietzmann
Copy link
Member

Okay. Maybe we should give him some time and try to restart later...

@PeterKietzmann
Copy link
Member

It was a long way. But finally things come to an end :-) . ACK and go

PeterKietzmann added a commit that referenced this pull request Sep 7, 2015
@PeterKietzmann
Merge pull request #2799 from ReneHerthel/arduino_2560_gpio
4e47e90 
Arduino-mega2560 GPIO implementation
@PeterKietzmann PeterKietzmann merged commit 4e47e90 into RIOT-OS:master Sep 7, 2015
@PeterKietzmann
Copy link
Member

@ReneHerthel congratulations for your first contribution!

@PeterKietzmann
Copy link
Member

@OlegHahm and sorry for merging that in FeatureFreeze. It was important due to motivational reasons ;-)

@ReneHerthel ReneHerthel deleted the arduino_2560_gpio branch September 8, 2015 11:41
@ReneHerthel ReneHerthel restored the arduino_2560_gpio branch September 8, 2015 11:41
@ReneHerthel ReneHerthel deleted the arduino_2560_gpio branch September 17, 2015 13:53
@LudwigKnuepfer
Copy link
Member

@ReneHerthel please document the pin layout in the wiki

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@PeterKietzmann PeterKietzmann

Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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
Projects
None yet
Milestone
Release 2015.12
Development

Successfully merging this pull request may close these issues.
6 participants
@ReneHerthel @PeterKietzmann @OlegHahm @kaspar030 @LudwigKnuepfer @haukepetersen