Skip to content

Conversation

A-Paul
Copy link
Member

@A-Paul A-Paul commented Aug 29, 2017

This is a preparation for implemeting a PWM driver for the atmeg328p MCU. The periph_conf has not been prepared for board specific configurations.

I just placed a periph_conf.h in the common include directory, which works fine. If it is copied to the board specific include directory then the make system selects that instead.

@miri64 for the board definitions you have placed a empty board.h in every specific include directory and none in the common. Are there reasons to prefer that schema?

@A-Paul A-Paul added Platform: AVR Platform: This PR/issue effects AVR-based platforms Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Aug 29, 2017
@A-Paul A-Paul requested review from miri64 and smlng August 29, 2017 15:15
@haukepetersen
Copy link
Contributor

Why not go all the way here? The common file already contains a lot of `#ifdef' blocks, so why not make them specific to each atmega based board right away? This would be much cleaner...

@A-Paul
Copy link
Member Author

A-Paul commented Aug 29, 2017

Why not go all the way here? ...

@haukepetersen because that would be a shitload of work for me. Just kidding...
I was already wondering if having those conditionals in "common" might signify it is "uncommon". ;)
I can try to clean this up. In the current state the arduino section is rather confusing to me. prefer to do further steps in separate PRs based on this.

Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

there is also the atmega1281 used in the waspmote-pro, which should be considered here, too, but is currently missing.

* @{
*/
#ifdef CPU_ATMEGA328P
#define TIMER_NUMOF (2U)
Copy link
Member

Choose a reason for hiding this comment

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

looks like there actually only 1 timer, right?

#define TIMER_0_FLAG &TIFR1
#define TIMER_0_ISRA TIMER1_COMPA_vect
#define TIMER_0_ISRB TIMER1_COMPB_vect
#define TIMER_0_ISRC TIMER1_COMPC_vect
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 simplified to this line by removing #ifdef CPU_ATMEGA328P above

@miri64
Copy link
Member

miri64 commented Nov 15, 2017

From the title I can't tell what this PR is about. Please update.

@miri64 miri64 removed their request for review November 15, 2017 18:39
@miri64 miri64 removed their assignment Nov 15, 2017
@A-Paul
Copy link
Member Author

A-Paul commented Jun 17, 2018

Closing because of already restructured boards/common tree.

@A-Paul A-Paul closed this Jun 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Platform: AVR Platform: This PR/issue effects AVR-based platforms State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants