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

Feat/nrf52xxx/adc/make resolution changeable #4701

Merged
merged 5 commits into from
Jan 20, 2025
Merged

Feat/nrf52xxx/adc/make resolution changeable #4701

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac8ca36
feat(src/machine/nrf52xxx/adc): move init to init function
milkpirate Jan 14, 2025
10e6124
chore(src/machine/nrf52xxx/adc): fix typo and formatting
milkpirate Jan 14, 2025
bfa1551
feat(src/machine/nrf52xxx/adc): make resolution configurable
milkpirate Jan 14, 2025
de4ef7a
fix(src/machine/nrf52xxx/adc): Register16.Get() already returns uint1…
milkpirate Jan 14, 2025
33e7cd0
chore(review): honor
milkpirate Jan 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 30 additions & 31 deletions src/machine/machine_nrf52xxx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,17 @@ func CPUFrequency() uint32 {

// InitADC initializes the registers needed for ADC.
func InitADC() {
return // no specific setup on nrf52 machine.
}

// Configure configures an ADC pin to be able to read analog data.
func (a ADC) Configure(config ADCConfig) {
// Enable ADC.
// The ADC does not consume a noticeable amount of current simply by being
// enabled.
// The ADC does not consume a noticeable amount of current by being enabled.
nrf.SAADC.ENABLE.Set(nrf.SAADC_ENABLE_ENABLE_Enabled << nrf.SAADC_ENABLE_ENABLE_Pos)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So now we going to have ADC automatically enabled on NRF chips. If the comment is true, then probably it's fine.
How can we confirm that? NRF chip is particularly good for projects there extreme low power consumption is required (hibernation). Will this change jeopardise this?

Copy link
Author

Choose a reason for hiding this comment

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

Well I would expect initialization code to be in a function called InitADC(), and not in Configure(). If(!) InitADC() is always called (maybe by tinygo somewhere), then yes, then the ADC will be always on. Further I do not know if the comment is correct 🤷🏻 probably @aykevl known.

If not it will only be turned on if one does InitADC(). I guess it will break every code that does not InitADC().

This poses the question: Whats valued more coherent code or not breaking existing one?

I am defiantly more leaning to the first, but this is not my party 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I did assume wrong, InitADC() is not called in runtime init method on NRF chips, like InitSerial() does, for example. See https://github.com/tinygo-org/tinygo/blob/release/src/runtime/runtime_nrf.go#L29-L33
And this example confirms that, client code need to call it explicitly: https://github.com/tinygo-org/tinygo/blob/release/src/examples/adc/adc.go#L12

Not sure at the moment if this is by design or simply inconsistency that we want to fix.
@aykevl

Copy link
Member

Choose a reason for hiding this comment

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

This is by design. It you are not using ADC then you don't have to bring it with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, why do we init serial unconditionally then?

Copy link
Member

@deadprogram deadprogram Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Several reasons, one of which is that without serial, you cannot use the auto-detect of 1200 baud for kicking into bootloader mode.

Copy link
Author

Choose a reason for hiding this comment

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

So leave it in? @deadprogram


// Use fixed resolution of 12 bits.
// TODO: is it useful for users to change this?
nrf.SAADC.RESOLUTION.Set(nrf.SAADC_RESOLUTION_VAL_12bit)
// Configure configures an ADC pin to be able to read analog data.
// Reference voltage can be 150, 300, 600, 1200, 1800, 2400, 3000(default), 3600 mV
// Resolution can be 8, 10, 12(default), 14 bits
// SampleTime will be ceiled to 3(default), 5, 10, 15, 20 or 40(max) µS respectively
// Samples can be 1(default), 2, 4, 8, 16, 32, 64, 128, 256 samples
func (a *ADC) Configure(config ADCConfig) {

var configVal uint32 = nrf.SAADC_CH_CONFIG_RESP_Bypass<<nrf.SAADC_CH_CONFIG_RESP_Pos |
nrf.SAADC_CH_CONFIG_RESP_Bypass<<nrf.SAADC_CH_CONFIG_RESN_Pos |
Expand All @@ -51,11 +49,26 @@ func (a ADC) Configure(config ADCConfig) {
case 3600: // 3.6V
configVal |= nrf.SAADC_CH_CONFIG_GAIN_Gain1_6 << nrf.SAADC_CH_CONFIG_GAIN_Pos
default:
// TODO: return an error
// TODO: return an error, will that interfere with any interfaced if one will be?
}

// Source resistance, according to table 89 on page 364 of the nrf52832 datasheet.
// https://infocenter.nordicsemi.com/pdf/nRF52832_PS_v1.4.pdf
var resolution uint32
switch config.Resolution {
case 8:
resolution = nrf.SAADC_RESOLUTION_VAL_8bit
case 10:
resolution = nrf.SAADC_RESOLUTION_VAL_10bit
case 12:
resolution = nrf.SAADC_RESOLUTION_VAL_12bit
case 14:
resolution = nrf.SAADC_RESOLUTION_VAL_14bit
default:
resolution = nrf.SAADC_RESOLUTION_VAL_12bit
}
nrf.SAADC.RESOLUTION.Set(resolution)

// Source resistance, according to table 41 on page 676 of the nrf52832 datasheet.
// https://docs-be.nordicsemi.com/bundle/ps_nrf52840/attach/nRF52840_PS_v1.11.pdf?_LANG=enus
if config.SampleTime <= 3 { // <= 10kΩ
configVal |= nrf.SAADC_CH_CONFIG_TACQ_3us << nrf.SAADC_CH_CONFIG_TACQ_Pos
} else if config.SampleTime <= 5 { // <= 40kΩ
Expand Down Expand Up @@ -102,36 +115,28 @@ func (a ADC) Configure(config ADCConfig) {
nrf.SAADC.CH[0].CONFIG.Set(configVal)
}

// Get returns the current value of a ADC pin in the range 0..0xffff.
func (a ADC) Get() uint16 {
// Get returns the current value of an ADC pin in the range 0..0xffff.
func (a *ADC) Get() uint16 {
var pwmPin uint32
var rawValue volatile.Register16

switch a.Pin {
case 2:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput0

case 3:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput1

case 4:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput2

case 5:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput3

case 28:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput4

case 29:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput5

case 30:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput6

case 31:
pwmPin = nrf.SAADC_CH_PSELP_PSELP_AnalogInput7

default:
return 0
}
Expand Down Expand Up @@ -164,13 +169,7 @@ func (a ADC) Get() uint16 {
}
nrf.SAADC.EVENTS_STOPPED.Set(0)

value := int16(rawValue.Get())
if value < 0 {
value = 0
}

// Return 16-bit result from 12-bit value.
return uint16(value << 4)
return rawValue.Get()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this break something? Like a client code out there in the wild expects 16bit value.

Copy link
Author

Choose a reason for hiding this comment

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

Well, honestly this whole block makes no sense to me:

  • Why convert a Uint to an int and then check if its <0?
  • Then value << 4 does not convert something from 12 to 16 bit, it multiplies the value by $2^4 = 16$. Why?

I may not be aware of some special cases why this was done like so, maybe @aykevl (the original author) can shed some light on this.

Copy link
Contributor

@ysoldak ysoldak Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the idea is (though I can't find this in documentation, probably picked up this from slack conversations) the value is always 16bit (i.e. ADC max is always 65535). But when hardware can't deliver that, we simply have less resolution. This way the client code does not need to worry about ADC MAX.

Copy link
Author

Choose a reason for hiding this comment

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

Ahhh! Ohhh!... Ok, well then the shift must depend on the resolution. Can add that if you like. Then it remain compatible with current usages.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, correct, shift depends on ADC resolution.

}

// SPI on the NRF.
Expand All @@ -196,7 +195,7 @@ type SPIConfig struct {
Mode uint8
}

// Configure is intended to setup the SPI interface.
// Configure is intended to set up the SPI interface.
func (spi SPI) Configure(config SPIConfig) error {
// Disable bus to configure it
spi.Bus.ENABLE.Set(nrf.SPIM_ENABLE_ENABLE_Disabled)
Expand Down
Loading