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

cpu/cc260x: add I2C implementation #11306

Merged
merged 5 commits into from
Apr 8, 2019
Merged

cpu/cc260x: add I2C implementation #11306

merged 5 commits into from
Apr 8, 2019

Conversation

MrKevinWeiss
Copy link
Contributor

Contribution description

Adds a previously missing periph/i2c implementation for TI cc26xx.

Testing procedure

Run the i2c robot-test and verify with manually with tests/periph_i2c or i2c based driver tests.

Issues/PRs references

Taken over from #10955

@MrKevinWeiss MrKevinWeiss added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Area: cpu Area: CPU/MCU ports labels Mar 28, 2019
@MrKevinWeiss
Copy link
Contributor Author

I still need to add protection for weird flags that could lock up the device.

I would also like to know if I should add a timeout or if we can assume the bits will toggle and while loops are fine?

smlng
smlng suggested changes Mar 29, 2019
View reviewed changes
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.

btw. PR title is a odd

cpu/cc26x0/periph/i2c.c Outdated Show resolved Hide resolved
cpu/cc26x0/periph/i2c.c Outdated Show resolved Hide resolved
cpu/cc26x0/periph/i2c.c Outdated Show resolved Hide resolved
cpu/cc26x0/periph/i2c.c Outdated Show resolved Hide resolved
cpu/cc26x0/Makefile.include Show resolved Hide resolved
@MrKevinWeiss
Copy link
Contributor Author

Many thanks, I will address them shortly

@MrKevinWeiss MrKevinWeiss changed the title Pr/addi2ctocc26xx cpu/cc260x: add I2C implementation Mar 29, 2019
@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet labels Mar 29, 2019
@MrKevinWeiss
Copy link
Contributor Author

Alright, I think it is in good shape, code style wise do you prefer while (...) {} or while (...) {};. Uncrustify says without semicolon.

@smlng
Copy link
Member

smlng commented Mar 29, 2019

oopsi, my bad according to our Coding Conventions w/o semicolon is correct

smlng
smlng suggested changes Mar 29, 2019
View reviewed changes
cpu/sam0_common/periph/i2c.c Outdated Show resolved Hide resolved
dist/tools/jlink/jlink.sh Outdated Show resolved Hide resolved
cpu/cc26x0/periph/i2c.c Outdated Show resolved Hide resolved
cpu/cc26x0/periph/i2c.c Outdated Show resolved Hide resolved
@MrKevinWeiss
Copy link
Contributor Author

@smlng Should I squash? If I do can I keep @kaspar030 messages?

kaspar030
kaspar030 reviewed Apr 1, 2019
View reviewed changes
cpu/cc26x0/periph/i2c.c Outdated
ret = -EAGAIN;
}
/*
* If an error if a non-NACK error occurs we must reinit or get stuck.
Copy link
Contributor

Choose a reason for hiding this comment

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

sentence starts weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh yup, race condition in my head I guess.

kaspar030
kaspar030 reviewed Apr 1, 2019
View reviewed changes
cpu/cc26x0/periph/i2c.c
/* enable SERIAL power domain */
PRCM->PDCTL0SERIAL = 1;
while (!(PRCM->PDSTAT0 & PDSTAT0_SERIAL_ON)) {}

Copy link
Contributor

Choose a reason for hiding this comment

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

extra newlines

kaspar030
kaspar030 reviewed Apr 1, 2019
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

minor style nits

@MrKevinWeiss
Copy link
Contributor Author

Thanks @kaspar030, sorry guys for the silly mistakes. I am maybe rushing it a bit.

@smlng
Copy link
Member

smlng commented Apr 8, 2019

please squash

smlng
smlng suggested changes Apr 8, 2019
View reviewed changes
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.

cannot test, board configs are missing

kaspar030 and others added 2 commits April 8, 2019 11:32
@kaspar030 @MrKevinWeiss
boards/cc2650-launchpad: configure i2c feature
44ecf0e
@MrKevinWeiss
cpu/cc26x0/i2c: Rework and add error handling
4c9890b 
This commit cleans up magic number and defines bitfields.
Adds error codes for ADDR/DATA NACK and ARBLOSS
Adds error handling, it corrects when an error occurs
Protects from flags that could lockup the bus
@MrKevinWeiss
Copy link
Contributor Author

Uggg, error described in #10958.
Should I fix it in this PR?

@smlng
Copy link
Member

smlng commented Apr 8, 2019

Uggg, error described in #10958.
Should I fix it in this PR?

I think so, in contrast to #10958 I would suggest to change the macro name in the drivers that fail. Looking in some other drivers, we mostly use BUS or DEV as short hand macros and not I2C, so if you change to either one the error should be gone.

@kaspar030
Copy link
Contributor

I think so, in contrast to #10958 I would suggest to change the macro name in the drivers that fail. Looking in some other drivers, we mostly use BUS or DEV as short hand macros and not I2C, so if you change to either one the error should be gone.

+1, much better than the #undef hack.

@MrKevinWeiss
drivers: adcxx1c, ads101x, ad7746: Change I2C macro to DEV
3ed9060 
Due to a name clash the helper macro I2C should be change.
Helper macros in other drivers are called DEV.
Changing to DEV fixes the naming conflict.
@MrKevinWeiss
Copy link
Contributor Author

Finally. Is everyone happy?

@smlng smlng added Type: new feature The issue requests / The PR implemements a new feature for RIOT Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines and removed Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Apr 8, 2019
@smlng smlng merged commit 7484af6 into RIOT-OS:master Apr 8, 2019
@MrKevinWeiss
Copy link
Contributor Author

Thanks for the reviews!

@MrKevinWeiss MrKevinWeiss deleted the pr/addi2ctocc26xx branch April 9, 2019 07:24
@kaspar030
Copy link
Contributor

Thanks again for taking over @MrKevinWeiss!

This was referenced Apr 9, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaspar030 kaspar030 kaspar030 left review comments

@smlng smlng smlng approved these changes

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MrKevinWeiss @smlng @kaspar030