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

samr21: implemention of transceiver via spi #1997

Merged
merged 1 commit into from
Dec 17, 2014

Conversation

Troels51
Copy link
Contributor

Implementaion of the at86rf231 driver for the samr21 chip this include

  • A gpio fix
  • An SPI implementation with SPI_0 being used for the internal transceiver. Does not support more than 1Mhz transfer

@OlegHahm OlegHahm added Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 11, 2014
@@ -0,0 +1,4 @@
ifneq (,$(filter defaulttransceiver,$(USEMODULE)))
USEMODULE += at86rf231
USEMODULE += transceiver
Copy link
Member

Choose a reason for hiding this comment

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

I think you just need to use the transceiver module if the netdev is not available. You could do something like this:

ifneq (,$(filter defaulttransceiver,$(USEMODULE)))
    USEMODULE += at86rf231
    ifeq (,$(filter netdev_base,$(USEMODULE)))
        USEMODULE += transceiver
    endif
endif

@haukepetersen
Copy link
Contributor

Hi @Troels51, first of all, very nice work! Sorry for putting quite a few comments in your code. I think the overall SPI driver is in a very good state, most remarks are rather minor structuring things.

I would suggest you put out the fixes to the GPIO driver as separate PR. Important to understand about the low-level driver concept is, that the SPI and GPIO parts should be completely independent. The SPI driver should only care about MISO, MOSI and SCLK pins. All other pins should be simply configured as standard GPIO devices for the GPIO driver. The radio driver will then take care of using these GPIOs...

SPI_0_SCLK_DEV.PMUX[(SPI_0_SCLK_PIN % 32) / 2].bit.PMUXE = 5;
SPI_0_MISO_DEV.PMUX[(SPI_0_MISO_PIN % 32) / 2].bit.PMUXO = 5;
SPI_0_MOSI_DEV.PMUX[(SPI_0_MOSI_PIN % 32) / 2].bit.PMUXE = 5;

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you can avoid the modulo- and division operations by definig distinct configurations in the periph_conf.h like @haukepetersen said

@PeterKietzmann
Copy link
Member

I really like this PR and will test it soon!


int spi_init_slave(spi_t dev, spi_conf_t conf, char (*cb)(char))
{
/* TODO */
Copy link
Member

Choose a reason for hiding this comment

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

Even if you did not implement the SPI slave, could you also "implement" spi_transmission_begin (like spi_init_slave) because the SPI test includes this..

@Troels51
Copy link
Contributor Author

Troels51 commented Dec 1, 2014

In cpu's with a different endian from the transceiver the short would be reversed right?
I found that my messages were getting filtered by the transceiver.. when i used HTONS this problem disappeared.

@thomaseichinger
Copy link
Member

How do you send? When interacting directly with the RIOT transceiver you have to specify the destination address in network byte order. (see _transceiver_send_handler in sys/shell/commands/sc_transceiver.c)

@thomaseichinger thomaseichinger removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Dec 1, 2014
@Troels51
Copy link
Contributor Author

Troels51 commented Dec 1, 2014

It came about when i was trying out the rpl_udp example, i could only receive multicasts before trying to reverse the network order. Could it be that the byte order of the address is reversed at another point and i'm just correcting it?

@maxencechotard
Copy link
Member

@Troels51 do you know how to use default exemple with shell functionnalities ? Have you ever managed to do this ?

@Troels51
Copy link
Contributor Author

Troels51 commented Dec 1, 2014

@maxencechotard Hmm, i just tried the default example (i should probably have tried it before) and it only works without reversing the byte orders, so reversing is not the solution.

But when trying the rpl_udp i can only get messages through when using HTONS on the addresses, so the problem must be somewhere else.

@biboc
Copy link
Member

biboc commented Dec 4, 2014

I post here about the GPIO and EIC. @Troels51 I tried the source code by configuring an interrupt on SW0. Here is what I did:
value = gpio_init_int(GPIO_1, GPIO_PULLUP , GPIO_BOTH, callback_test,NULL);
gpio_irq_enable(GPIO_1);
Am I doing right? Because the pull-up doesn't seem to be right set (TP300 is not 1)
The callback "works" but plenty of time instead of only when I press the button.

@biboc
Copy link
Member

biboc commented Dec 4, 2014

Ok I found your error.
In gpio_init_int, you use gpio_init_in to configure the pin as an input. Then you use WRCONFIG in gpio_init_int and you write WRPINCFG to 1 which "update the Pin Configuration register (PINCFGy)" so all your previous config are set to 0.
And then the pull up is disable and it was why I observed a weird behaviour of my callback ;-)

You just need to call :
/* configure pin as input */
res = gpio_init_in(dev, pullup);
if (res < 0) {
return res;
}
after WRCONFIG and it works.

@thomaseichinger
Copy link
Member

@Troels51 please address @bapclenet's comment and rebase so we can merge this one.

@saurabh984
Copy link

@Troels51 Could really do with this merge. Let me know if there's anything I could do to help. I have 15 of these boards lying around gathering dust.

@biboc
Copy link
Member

biboc commented Dec 15, 2014

@Troels51, I found another weird behaviour.
Set the switch as an interruption, try it for a while and after 15sec, set it as disable (the interrupt doesn't work which is fine), then set it back to enabled, at this moment when INTENSET bit is set to 1, an interrupt is raised which is not expected as I didn't press the button. Do you see what I mean?

Even if it works after enable it for the second time, the interrupt which is raised after enabling the interruption shouldn't occur. What do you think about that?

PS: I've got the same problem with RTC's interruption

@Troels51 Troels51 force-pushed the samr21-transceiver-port branch from f8d9111 to bff54e6 Compare December 15, 2014 14:37
@@ -463,8 +465,8 @@ int gpio_init_int(gpio_t dev, gpio_pp_t pullup, gpio_flank_t flank, gpio_cb_t cb
gpio_config[extint].arg = arg;

Copy link
Member

Choose a reason for hiding this comment

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

Clear the interrupt flag of the given GPIO:
EIC->INTFLAG.reg = EIC_INTFLAG_EXTINTX;

@biboc
Copy link
Member

biboc commented Dec 15, 2014

Ok, I found how to solve your problem:
Clear the interrupt flag of the given GPIO before EIC->INTENSET.reg = (1 << (extint));
EIC->INTFLAG.reg = EIC_INTFLAG_EXTINTX;

@Troels51
Copy link
Contributor Author

@bapclenet I implemented the change you suggested, do you think that solves the problem you were having? it seems weird that it is connected with the RTC

@Troels51
Copy link
Contributor Author

@saurabh984 Sorry for being a bit slow, i've been focused on some school work. You could pull the changes from my branch directly to see how it works.

@biboc
Copy link
Member

biboc commented Dec 15, 2014

Yes that was the same problem. They are not connected but the interruption system is similar in both component. (INTFLAG,INTENTSET and INTENTCLR)

@Troels51
Copy link
Contributor Author

So you have to clear the flag before you can enable the interrupt?

@biboc
Copy link
Member

biboc commented Dec 15, 2014

Yes, the same as you. I don't know why the flag is set to 1 before renabling the interrupt but this solution works.

@Troels51 Troels51 force-pushed the samr21-transceiver-port branch from 9f332a3 to 449c4dd Compare December 16, 2014 15:19
@thomaseichinger
Copy link
Member

@Troels51 could you address @N8Fear's #1997 (comment) and squash? I'd say we are good to go then. Travis failing for stm32f0discovery and riot_and_cpp seems unrelated.

@Troels51 Troels51 force-pushed the samr21-transceiver-port branch from 88c2e05 to 33ef43c Compare December 16, 2014 16:43
@Troels51
Copy link
Contributor Author

Added the newline and squashed the commmits

@thomaseichinger
Copy link
Member

Perfect. could you rebase one last time to include #2204. Just to be sure.

@thomaseichinger
Copy link
Member

Travis passed, code looks good, tested and works. Let's do this. ACK & Go

thomaseichinger added a commit that referenced this pull request Dec 17, 2014
samr21: implemention of transceiver via spi
@thomaseichinger thomaseichinger merged commit 5689a7d into RIOT-OS:master Dec 17, 2014
@thomaseichinger
Copy link
Member

Congratulations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Platform: ARM Platform: This PR/issue effects ARM-based platforms 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.

9 participants