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

board/Nucleo-f303: initial support for the Nucleo-f303 #3165

Merged
merged 1 commit into from
Jun 16, 2015

Conversation

katezilla
Copy link
Contributor

Add support for Nucleo-f303 (STM32f303)

@katezilla katezilla changed the title Nucleo-f303 initial commit Add support for Nucleo-f303 Jun 4, 2015
@BytesGalore BytesGalore changed the title Add support for Nucleo-f303 board/Nucleo-f303: initial support for the Nucleo-f303 Jun 4, 2015
@BytesGalore BytesGalore added Type: new feature The issue requests / The PR implemements a new feature for RIOT Platform: ARM Platform: This PR/issue effects ARM-based platforms labels Jun 4, 2015
@BytesGalore
Copy link
Member

Congrats for your first PR :)

include $(RIOTBOARD)/Makefile.include.openocd

# include cortex defaults
include $(RIOTBOARD)/Makefile.include.cortexm_common
Copy link
Member

Choose a reason for hiding this comment

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

please add a newline here

/**
* @name Define the nominal CPU core clock in this board
*/
#define F_CPU CLOCK_CORECLOCK
Copy link
Member

Choose a reason for hiding this comment

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

please add brackets (also for UART_0 and TIMER_0 below)

Copy link
Member

Choose a reason for hiding this comment

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

ok forget this, it seems we never used brackets for these cases.

@BytesGalore BytesGalore added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 4, 2015
@BytesGalore
Copy link
Member

alright, the white-space warnings are history 👍
I think you can squash your commits now, then lets see what @PeterKietzmann says.

* @{
**/
#define CLOCK_HSE (8000000U) /* external oscillator */
#define CLOCK_CORECLOCK (72000000U) /* desired core clock frequency */
Copy link
Contributor

Choose a reason for hiding this comment

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

the indention seems to be of by a few spaces 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.

less spaces or more for it to be on same collum as the other comments?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would put it to the same indention as the lines below.

@haukepetersen
Copy link
Contributor

Nice one! Except some minor style issues I think the PR looks good. Codewise you get my ack once the comments above are addressed. I don't have the board here, @BytesGalore and @PeterKietzmann, will you test it?

Small thing when squashing: try to give your commits some good names. The convention used by most is something like MAIN_MODULE/MODULE: this is what I did, e.g. boards: added support for the nucleo-f303 or something like this...

*/

#ifndef __PERIPH_CONF_H
#define __PERIPH_CONF_H
Copy link
Member

Choose a reason for hiding this comment

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

Change this include guard to PERIPH_CONF_H_ instead. Double underscores are reserved.

@PeterKietzmann
Copy link
Member

@katezilla, looks good :-) ! Congrats for your first PR. Please address the minor style comments and just ask if something is unclear.

@PeterKietzmann
Copy link
Member

@katezilla thanks for the adaptions. Could you also address the newline-comments please? Otherwise we're not far from merging this PR :-)

@PeterKietzmann
Copy link
Member

Sorry, now I see you already addressed the newline comments. Could you please add an I2C configuration to the periph_conf as a last step :-) ?

@PeterKietzmann
Copy link
Member

@katezilla I tested I2C. For some reasons I2C_0 works and I2C_1 does not. Any ideas? Anyway, I think you can already squash your commits to one. I think your first commit message is fine.

@PeterKietzmann
Copy link
Member

Does anyone have a stm32f3discovery board and can test the I2C_1 driver?

@PeterKietzmann
Copy link
Member

@katezilla you defined pins PD12...15 for the PWM_1_DEV. Where do I access them on the board. Is this schematic correct?

@PeterKietzmann
Copy link
Member

I did the following measurement with the settings:

  • frequency = 1000 Hz
  • duty-cycle = 10us
    bildschirmfoto vom 2015-06-12 09_24_21
    It appears that the period time and the duty cycle are half as long as expected. Seems like a clock configuration thing. @haukepetersen do you know where the bug is or is it maybe in the driver? Can someone confirm with an stm32f3discovery

@haukepetersen
Copy link
Contributor

Just did. It's a bug in the frequency calculation of the f3 pwm driver -> unrelated to this PR. Lets just merge this board as is, but please open an issue so we can fix it afterwards for all f3 boards!

@PeterKietzmann
Copy link
Member

ACK! I'll merge when Travis is green. Jippie!

@haukepetersen haukepetersen removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Jun 12, 2015
@haukepetersen
Copy link
Contributor

removed squashing label and restarted Travis

@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 12, 2015
@PeterKietzmann
Copy link
Member

The cortex-m4 build fails on the ported nucleo-f303 board for the both tests driver_at86rf2xx and driver_isl29125. Can anyone imagine why?

@jnohlgard
Copy link
Member

jnohlgard commented Jun 15, 2015 via email

@PeterKietzmann
Copy link
Member

That would make sense cause the one test requires GPIO_5 and the other GPIO_0 ... GPIO_4. In this PR we just introduced the GPIOs GPIO_0 ... GPIO_2. I'd prefer to merge this PR quickly without many (big) extensions. Do we have some kind of application blacklist? I don't know...

@jnohlgard
Copy link
Member

jnohlgard commented Jun 15, 2015 via email

@haukepetersen
Copy link
Contributor

Actually, the GPIO issue should be obsolete as #3095 is merged. To adapt this PR simply rebase this PR on master and remove the complete GPIO section from the periph_conf.h. Then Travis should run through just fine.

@PeterKietzmann
Copy link
Member

@haukepetersen do I see it correctly that just the GPIO_NUMUF needs to be removed for adaptation to #3095?

@PeterKietzmann
Copy link
Member

ACK and go. Will do a PR to remove the superfluous macro

PeterKietzmann added a commit that referenced this pull request Jun 16, 2015
board/Nucleo-f303: initial support for the Nucleo-f303
@PeterKietzmann PeterKietzmann merged commit 666ad5d into RIOT-OS:master Jun 16, 2015
@PeterKietzmann
Copy link
Member

Congrats for your first PR @katezilla!

@katezilla
Copy link
Contributor Author

thx :)

@katezilla katezilla deleted the nucleo-f303 branch June 25, 2015 14:24
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.

5 participants