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, cc2538: add low-level adc periph driver #7327

Merged
merged 3 commits into from
Jul 27, 2017
Merged

cpu, cc2538: add low-level adc periph driver #7327

merged 3 commits into from
Jul 27, 2017

Conversation

smlng
Copy link
Member

@smlng smlng commented Jul 6, 2017 •
edited
Loading

started from #5823 (and makes that obsolete), but rewrote ADC based on TI periphal driver library. Adds also board periph confs for all CC2538 based boards, including SAUL adaption for Zolertia Remotes.

Requires #7320 (and all sub PRs dependencies of that one).

@smlng smlng added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 6, 2017
@smlng smlng added this to the Release 2017.10 milestone Jul 6, 2017
@smlng smlng self-assigned this Jul 6, 2017
@smlng smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 6, 2017
@smlng smlng requested review from PeterKietzmann, A-Paul and a user July 6, 2017 12:07
@kYc0o
Copy link
Contributor

kYc0o commented Jul 6, 2017

I see some commits which at a first sight don't belong to thus PR. Something wrong with the rebase?

@smlng
Copy link
Member Author

smlng commented Jul 6, 2017

as stated above, is based on several of my recent cc2538 rework/refine PRs - but some other commits slipped also in yes. At least #7316 needs to be merged first, afterwards I'll rebase as needed ...

So, if anyone wants to help testing please start with #7316!

ghost
ghost suggested changes Jul 9, 2017
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Somehow, ADCs on the Re-Mote board do not work properly. ADC pins do not match with the terminal output. In addition, low level is displayed incorrectly (> 4000). Tested with "tests/periph_adc", 12 bit resolution.

boards/remote-pa/include/adc_params.h Outdated
.res = ADC_RES_12BIT,
}
};
#ifdef __cplusplus
Copy link

Choose a reason for hiding this comment

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

Blank line?

boards/remote-revb/include/adc_params.h Outdated
.res = ADC_RES_12BIT,
}
};
#ifdef __cplusplus
Copy link

Choose a reason for hiding this comment

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

Blank line?

cpu/cc2538/include/periph_cpu.h Outdated
#define SOC_ADC_ADCCON3_ECH 0x0000000F /**< Single channel select */
/** @} */

/** @name SOC_ADC_ADCCONx registers field values
Copy link

Choose a reason for hiding this comment

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

Line break?

cpu/cc2538/include/periph_cpu.h Outdated
*/
#define SOC_ADC_ADCCON_REF SOC_ADC_ADCCON_REF_INT

/** @name SOC_ADC_ADCCON3 register bit masks
Copy link

Choose a reason for hiding this comment

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

Line break?

@smlng
Copy link
Member Author

smlng commented Jul 10, 2017

Fixed the miss matched ADC lines, however, the output looks still a bit weird. That is, with 10Bit resolution applying HIGH on any channel results in 511 and applying GND result in 0 on ADC3 (as expected) but 1023 on ADC1 and ADC2.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 10, 2017
@smlng
Copy link
Member Author

smlng commented Jul 10, 2017

btw. rebased onto #7316, to make merge easier

@smlng
Copy link
Member Author

smlng commented Jul 17, 2017 •
edited
Loading

rebased on (and includes) #7373

@smlng smlng added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 17, 2017
@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 19, 2017
@smlng
Copy link
Member Author

smlng commented Jul 19, 2017

this one would be ready - for testing and merging, that is 😉 On the remotes the actual values are a bit weird, which means with 10Bit resolution ADC1 reads 1020-1023 but ADC3 reads 0-2 when applying a LOW (i.e. GROUND) as expected. And both read 511 when applying a HIGH (i.e. VCC3.3). So if someone has a different board (openmote or cc2538dk) for testing would be great!

smlng
smlng commented Jul 20, 2017
View reviewed changes
cpu/cc2538/include/periph_cpu.h
*/
#define HAVE_ADC_RES_T
typedef enum {
ADC_RES_6BIT = (0xa00), /**< not supported by hardware */
Copy link
Member Author

Choose a reason for hiding this comment

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

@A-Paul this defines are used in similar fashion throught RIOTs ADC low-level drivers. I don't know why its done this way, but this here is consistent with other drivers.

@smlng
boards: adapt ADC config of cc2538 based boards
bfae64d 
adapted boards are: cc2538dk, openmote-cc2538, remote-pa,
remote-reva, and remote-revb.
@smlng
saul, adc: add config for remote boards
2806484
@smlng
Copy link
Member Author

smlng commented Jul 20, 2017

@A-Paul can you please test again with your cc2538dk board - I think I fixed it, though its a bit hacky as RIOT does not support differential ADC readings (yet).

A-Paul
A-Paul approved these changes Jul 21, 2017
View reviewed changes
Copy link
Member

@A-Paul A-Paul left a comment

Choose a reason for hiding this comment

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

After the last tweaks the output on a cc2538dk shows 1023 for a input voltage equal to VDD.
Dividing the voltage 1:1 with a resistor network gives ~505. So, the readout is almost sane.
I appreciate the comments giving a hint to the irregularities.
ACK regarding this board.

@smlng
Copy link
Member Author

smlng commented Jul 21, 2017

@dnahm if you can spare some time on testing to verify this, too - would be great thanks!

@A-Paul
Copy link
Member

A-Paul commented Jul 25, 2017

I tested with tests/periph_adc on a *remote-reva. Voltages taken from Pins 9,10,31.
"ADC1" ADC_LINE(0) and "ADC2" ADC_LINE(1) are returning a values of [0,1023] for [GND,3.3V].

"ADC3" ADC_LINE(2) returns [28,392,578] at [GND,3.3V,5.0V]. Considering the 28 as 0V reference this would make ~9.3V full scale.
@alignan, can you confirm this behavior?

@smlng
Copy link
Member Author

smlng commented Jul 25, 2017

as said offline, there is some voltage divider (or so) on ADC3 which make this ADC behave different from ADC1 and ADC2 - I don't know/remember the details right now, maybe @PeterKietzmann can shed some light?

@alignan
Copy link
Contributor

alignan commented Jul 25, 2017

Hello! sorry I have been absent, new job and less bandwidth...
ADC3 has a voltage divider, in the data sheet: ADC3 analogue input channel with 5/3 voltage divider to connect 5VDC-based analogue sensors

@smlng
Copy link
Member Author

smlng commented Jul 26, 2017 •
edited
Loading

@alignan so taking @A-Paul values for ADC3 with 5V = 578 and a voltage divider of 5/3, it is reasonable to convert like 578 / 3 * 5 = 963 <- considering that applied 5V might not be exact and/or ADC might above [be off] a bit 963 is close to the expected max of 1023 for 10Bit resolution right?

However, I would have expected, that applying 5V would give max value (1023) directly.

@alignan
Copy link
Contributor

alignan commented Jul 26, 2017

I seem to recall there was a change in the impedance value for the ADC from revA to revB, as the revA had a 300K/500K network divider vs the 30K/50K of the newest revB, due to the problem you now observe (impedance was too high). Could you check with a newer RE-Mote revision?

Adding also @amejias (from Zolertia) as they might be more available to test

@smlng
Copy link
Member Author

smlng commented Jul 27, 2017

Still, the ADC3 issue should not block this driver in general. That is better to have a mostly working ADC driver than none, at all 😉 right?

@smlng
Copy link
Member Author

smlng commented Jul 27, 2017

btw. @alignan

Could you check with a newer RE-Mote revision

@smlng smlng closed this Jul 27, 2017
@smlng smlng reopened this Jul 27, 2017
@smlng
Copy link
Member Author

smlng commented Jul 27, 2017

oops, wrong button ... need ☕️

anyway, what I meant: we test with remote revb here

@alignan
Copy link
Contributor

alignan commented Jul 27, 2017

Agree, the driver is in a better shape than it were on the original PR, happy with the results :-)

@smlng
Copy link
Member Author

smlng commented Jul 27, 2017

better shape than it were on the original PR

thanks for the 💐 - so I'll merge this, but we still should clarify the results for ADC3.

@smlng
Copy link
Member Author

smlng commented Jul 27, 2017

specifically the weird readings only apply for the remotes and ADC3 there on, cc2538dk seems on affected and for openmote-cc2538 we need test results from someone with the board available.

@smlng smlng merged commit 027d65d into RIOT-OS:master Jul 27, 2017
@smlng smlng mentioned this pull request Aug 30, 2017
Closed
@smlng smlng deleted the cpu/cc2538/periph_adc branch February 6, 2018 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A-Paul A-Paul A-Paul approved these changes

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

@smlng smlng

Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2017.10
Development

Successfully merging this pull request may close these issues.
4 participants
@smlng @kYc0o @A-Paul @alignan