Correcting ADMUX selection in __analogRead()

dbuezas / lgt8fx

Board Package for Logic Green LGT8F328P LGT8F328D and LGT8F88D

362 stars 87 forks source link

Correcting ADMUX selection in __analogRead() #130

Closed LaZsolt closed 2 years ago

LaZsolt commented 3 years ago

ADMUX input source used a wrong mask. (was 0x0f instead 0x1f)
LGT8F328D ADMUX has less input than P version. If someone want to read these unselectable inputs, it will return zero ADC value.
V5D1 and VCCM is same input with different name

frizzle101101 commented 3 years ago

is this why I'm getting 12bit default adc on projects

dbuezas commented 3 years ago

Hey @LaZsolt ! This will be a breaking change, right? (10bt analg read by default) I'll then release an intermediate version before, and then another one with this as v2. Do you mind adding a small example sketch to switch between analogRead resolutions?

LaZsolt commented 3 years ago

This will be a breaking change, right?

Yes, but as @jayzakk mentioned in https://github.com/dbuezas/lgt8fx/issues/41 the aduino default is 10 bits for backward compatibility with AVR based boards.

I think not necessary any example sketch, because it is a normal Arduino analog API extended function for those boards which have 12 bits ADC. Documentation: https://www.arduino.cc/en/Reference.AnalogReadResolution

It may be unpleasant for those users who will be upgrading dbuezas/lgt8fx core.

dbuezas commented 3 years ago

Gotcha. I'll make a release without, and then merge this one and move to v2 following semver

LaZsolt commented 3 years ago

But this request is not mainly about AnalogReadResolution.

In this line below, because of the bitmask of pin variable, only 8 analog inputs reachable. (A0 - A7) https://github.com/dbuezas/lgt8fx/blob/97e273aa27bd71ec168c2fa569a567edd3600abe/lgt8f/cores/lgt8f/wiring_analog.c#L147

With my correction more inputs will be readable: PIN_A8, PIN_A9, PIN_A10, PIN_A11, IVREF, AGND, DACO, 1/5 AVCC, 4/5 AVCC

dbuezas commented 3 years ago

Oh, so this is not breaking change then. Should i merge now?

LaZsolt commented 3 years ago

This pull request is two in one. 1st - expanding ADMUX pin select bitmask from 0x07 to 0x1F 2nd - changing the default resolution of analogRead() to 10 bits for Arduino compatibity reason.

So to get back the LGT8Fx 12-bit ADC capabilities, must change the resolution to 12 with AnalogReadResolution(12);. (At least in the setup function.)

I think it's acceptable. If you think the same then you should merge it.

dbuezas commented 3 years ago

Oh got it. Then Ill hold it a bit and very soon make 2 releases. The first without and the second with (and that will be v2.0.0)

navr32 commented 3 years ago

So this is the problem says why my A8 pin of wavgat board reject at build ? Arduino : 1.8.15 (Linux), Carte : "LGT8F328, Default (64), Internal, 32 MHz, 328P-LQFP32 (e.g. MiniEVB nano-style and WAVGAT)"

'A8' was not declared in this scope

LaZsolt commented 3 years ago

@navr32 In LGT8F328P-LQF32 the A8, A9 and A11 analog inputs not connected to any pins. That's why not declared. I assume the A8 name is wrong on the WAVGAT board. The real name coud be A6.

WAVGAT pro mini front-1

navr32 commented 3 years ago

Hi LaZolt ! You assume very well ! I just come to test it and this is A6 so simple ...very annoying i haven't think to this..many thanks.

LaZsolt commented 2 years ago

Close this request, because https://github.com/dbuezas/lgt8fx/pull/217 include the solution.