Skip to content

Conversation

@aabadie
Copy link
Contributor

@aabadie aabadie commented Apr 10, 2017

And factorize code with arduino-mkr1000 since these boards are very similar.
The Arduino MKRZERO uses the second SPI to connect a SD card device, I configured it to use the one available in RIOT. This is not yet tested though.

I also noticed that there's a third one in the family: Arduino MKRFOX1200 that offers a SigFox compatible antenna.

@aabadie aabadie added 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 Apr 10, 2017
@aabadie aabadie added this to the Release 2017.04 milestone Apr 10, 2017
@aabadie aabadie added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 10, 2017
@aabadie
Copy link
Contributor Author

aabadie commented Apr 10, 2017

And this PR is based on #6666

@aabadie aabadie force-pushed the mkrzero branch 3 times, most recently from 8411e1b to 3027ae1 Compare April 18, 2017 12:15
@aabadie
Copy link
Contributor Author

aabadie commented May 16, 2017

since #6666 has been merged, I rebased this one. Tested mkr1000, still works.

@aabadie aabadie removed the State: waiting for other PR State: The PR requires another PR to be merged first label May 16, 2017
* @name SD Card device configuration
* @{
*/
#define SDCARD_SPI_PARAM_SPI (SPI_DEV(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.

This is the part, I'm not totally sure of. @haukepetersen or @vincent-d any comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on what the question is... Does the arduino-mkrzero come with an on-board SD-card slot? If yes, then this looks valid to me (as long as the SD-card slot is connected to the pins mapped for SPI_DEV(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 should be ok, regarding the specs. I just don't know what CD1 is on the SD card reader.

Copy link
Member

Choose a reason for hiding this comment

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

Could be "card detect"?

Copy link
Member

Choose a reason for hiding this comment

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

Could be "card detect"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, googling points toward this direction. I added a define for this pin below in the file.

@haukepetersen
Copy link
Contributor

before merging this PR, I would however like to see a better solution for the bossa tool, as having the 4th board shipped with the same binary tool is ugly and bad -> so I would like to have a better solution before we go on. Sounds fair?

@aabadie
Copy link
Contributor Author

aabadie commented May 16, 2017

Sounds fair?

Yes, I'm interested in looking into this except if you feel very motivated.

@haukepetersen
Copy link
Contributor

done: #7068

.clk_pin = GPIO_PIN(PA, 13),
.miso_mux = GPIO_MUX_D,
.mosi_mux = GPIO_MUX_D,
.clk_mux = GPIO_MUX_D,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed my silly mistake while testing WINC1500 WiFi module. 🤕 Could you please change the MUX pads to:

.miso_mux = GPIO_MUX_C,
.mosi_mux = GPIO_MUX_C,
.clk_mux  = GPIO_MUX_C,

Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem, directly squashed and pushed

@aabadie aabadie force-pushed the mkrzero branch 2 times, most recently from 32ab02a to 65dcbd4 Compare May 20, 2017 16:15
},
{
.dev = &SERCOM2->SPI,
.miso_pin = GPIO_PIN(PA, 15),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pin definition for the second SPI is different from mkr1000. I'll have to provide the SPI configuration for each boards.

@haukepetersen
Copy link
Contributor

now that #7068 is merged, would you mind to rebase and make use of it?

@aabadie
Copy link
Contributor Author

aabadie commented May 23, 2017

would you mind to rebase and make use of it?

I'm on it

@aabadie aabadie force-pushed the mkrzero branch 2 times, most recently from 5b3281a to b43879e Compare July 15, 2017 14:34
@aabadie
Copy link
Contributor Author

aabadie commented Jul 17, 2017

@astralien3000, I pushed a fix regarding the PWM: pin ~2 and ~3 are working but I couldn't find the correct conf for pin ~4 and ~5. So there's only 2 available PWM pins in the default configuration.

Regarding the periph_timer test, I didn't test but it should work as I didn't change it and is working (normally) with mkr1000.

@aabadie
Copy link
Contributor Author

aabadie commented Aug 21, 2017

@astralien3000, if you have some time can you give another try to this one ?

@astralien3000
Copy link
Member

  • periph_pwm OK (for ~2 and ~3)
  • periph_timer still blocking

@aabadie aabadie force-pushed the mkrzero branch 2 times, most recently from fac316c to ad88779 Compare August 22, 2017 07:54
@aabadie
Copy link
Contributor Author

aabadie commented Aug 22, 2017

@astralien3000, thanks for testing again.

Regarding the timer test issue, I fixed it: by default mkrzero use the sdcard_spi module which itself uses xtimer and for some reason this messes up this test (TIMER_0 being initialized with different values apparently).
The fix is simply to remove the dependency to sdcard_spi which makes sense since this is up to the user to include it or not.

In the mean time, I also fixed the second spi periph configuration which was wrong.

Directly squashed and rebased.

Can you test again ?

@astralien3000
Copy link
Member

Yes ! Now it works !

@aabadie
Copy link
Contributor Author

aabadie commented Aug 22, 2017

@haukepetersen, this one is ready now (and tested). Would be great if you could approve it (and merge ;)) !

@aabadie aabadie force-pushed the mkrzero branch 2 times, most recently from 9e6168d to 4c35afe Compare August 22, 2017 14:47
@aabadie
Copy link
Contributor Author

aabadie commented Aug 22, 2017

@haukepetersen ?

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

Some minor things left, almost there :-)

#define ARDUINO_PIN_A5 GPIO_PIN(PA, 6) /* AIN6 */
#define ARDUINO_PIN_A6 GPIO_PIN(PA, 7) /* AIN7 */
/** @} */
/** @ */
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for removing the }?


/**
* @name Mapping of MCU pins to Arduino pins
* @name Mapping of MCU pins to Arduino pins
Copy link
Contributor

Choose a reason for hiding this comment

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

Indention off by one space.

*/
#define XTIMER TIMER_0
#define XTIMER_CHAN (0)
/** @} */
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this block - no need to re-define the default configuration

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2016 Inria
* Copyright (C) 2017 Inria
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think convention is to use 2016-2017 when editing. Applies to many files of this PR

@aabadie
Copy link
Contributor Author

aabadie commented Aug 25, 2017

@haukepetersen, comments addressed.

Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

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

ACK

@haukepetersen
Copy link
Contributor

all green -> go

@haukepetersen haukepetersen merged commit 191f38b into RIOT-OS:master Aug 25, 2017
@aabadie aabadie deleted the mkrzero branch February 26, 2018 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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.

7 participants