Feat/nrf52xxx/adc/make resolution changeable

[milkpirate]

As requested per #4660 (review)

What I did

ADC

• Change ADC methods to pointer receivers
• Make ADC resolution adjustable
• Remove nonsensical calculations (at least to me they seemed like, please correct me if I'm wrong)
• Update ADC documentation reference

Open questions

• Will make sense to change the signature of ADC.Get() to return an error? Since it will change the interface and might break things.

[ysoldak]

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

[milkpirate]

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.

[ysoldak]

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.

[milkpirate]

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.

[ysoldak]

Yes, correct, shift depends on ADC resolution.

[ysoldak]

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?

[milkpirate]

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 😄

[ysoldak]

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

[deadprogram]

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

[ysoldak]

Hm, why do we init serial unconditionally then?

[deadprogram]

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

[milkpirate]

So leave it in? @deadprogram

[milkpirate]

The switch case can be replaced by:

	// max precision 14bit (<<= 2, for 16 bit value)
	// min precision 8bit (<<= 8, for 16 bit value)
	// resolution constants 8bit ~ 0, 14bit ~ 3
	precisionAdjustment := 8 - 2*nrf.SAADC.RESOLUTION.Get()
        rawValue <<= precisionAdjustment

its more compact, but also "magical". Just tell me what you prefer.

[ysoldak]

I like this simple linear function conversion instead of a multiline switch 👍

[milkpirate]

So leave it in? @deadprogram

[ysoldak]

Looks fine to me.
Not 100% onboard with (unnecessary) pointer receiver change, but also have nothing against.
Feel free do 'magic' conversion for resolution adjustment, but it is fine with switch too.
Ready to approve once PR is not a draft anymore.