search

lancaster-university / codal-mbedos

1 stars 5 forks source link

ADC read on light sensor pin on brainpad returns out of range values #6

Open pelikhan opened 6 years ago

pelikhan commented 6 years ago

It seems that the scaling for the analog value at https://github.com/lancaster-university/codal-mbedos/blob/master/source/MbedPin.cpp#L312 does not work for the light sensor on BrainPad. The values returned are in the 16k range.

pelikhan commented 6 years ago

@jamesadevine @finneyj

jamesadevine commented 6 years ago

Yes, I think the way it's configured at the moment should have a range from -4096 to +4095, not 16k?

https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/TARGET_STM32F0/analogin_device.c#L73 is the line that needs changing, I think.

jamesadevine commented 6 years ago

@mmoskal @VictorDuma

jamesadevine commented 6 years ago

Also - here: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_STM/analogin_api.c#L34

Which is why you're seeing crazy values @pelikhan

pelikhan commented 6 years ago

@greg-norris we need your fixed sensor value in codal-brainpad to fix the lightsensor/temp sensor.

VictorDuma-zz commented 6 years ago

.getAnalogValue() temperature sensor about 4300. light sensor about 16000

2018-02-08 7:13 GMT-08:00 Peli de Halleux notifications@github.com:

@greg-norris https://github.com/greg-norris we need your fixed sensor value in codal-brainpad to fix the lightsensor/temp sensor.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lancaster-university/codal-mbedos/issues/6#issuecomment-364142147, or mute the thread https://github.com/notifications/unsubscribe-auth/AP7jjTBCiCkgbbf2pOqV9HALOs5cy2jDks5tSw8vgaJpZM4R9ooz .

mmoskal commented 6 years ago

@jamesadevine what STM is doing is correct - see https://os.mbed.com/docs/v5.7/mbed-os-api-doxy/classmbed_1_1_analog_in.html#a78a2d1c60bd542bacb785b6cc0701013

"16-bit unsigned short representing the current input voltage, normalised to a 16-bit value"

If nrf52 does something else, then it needs fixing and codal should always scale by 6 bits if we want 10 bit result.

jamesadevine commented 6 years ago

@mmoskal mbed appears to be wildly inconsistent... I think the solution here is to apply patches based on target / processor as hinted in my response to @greg-norris 's PR. i.e. subclass MbedPin, and override getAnalogValue and perform the appropriate normalisation.

@finneyj suggests we always normalise to an unsigned 16-bit integer.

Thoughts?

mmoskal commented 6 years ago

If the NRF52 doesn't return 16 bit value from read_u16, then it should be fixed, not STM.

jamesadevine commented 6 years ago

@mmoskal Sorry for the confusion. Let me clarify and summarise, as I wasn't clear before.

This issue was filed because @pelikhan was seeing values not normally expected, nor seen on any of our other platforms. My knee jerk reaction was to treat STM as an outlier, when in fact our other platforms using mbed are the outliers; my reaction was incorrect as you have identified. However, this tells us two things: (1) mbed os has inconsistencies in the values returned from read_u16() (2) based on our previous experiences with values returned from read_u16() our historic API for the Pin interface is 10-bit.

This means things need to change moving forward:

  1. Decide whether we would like to move to an properly unsigned, scaled, 16-bit representation in MbedPin. (I think yes)
  2. If yes, make the appropriate adjustments to documentation and code. Any outliers will have to subclass MbedPin and apply the appropriate transformation to meet the API (i.e. an unsigned 16-bit number). We should also add an API to configure the resolution of the ADC if we select this option.
  3. If no, and we want to keep the 10-bit representation, brainpad should subclass MbedPin and apply the appropriate transformation.
mmoskal commented 6 years ago

I guess it will be some work with the constants in analog sensors etc.

You could probably have a DEVICE_CONFIG constant for the required shift (defaulting to 0) - it would be easier than subclassing.

@pelikhan would we keep 0-1023 in CPX/Maker or go with the 0-65535?

gus-ghielec commented 6 years ago

We solve this on our OS by returning a double in the range 0 to 1. If you do not want to use double, scale all values to 16 bits. The problem is this is a needed change in mbed's core.