owntech-foundation / Core

A comprehensive API for power electronics based on Zephyr RTOS
https://www.owntech.org/
GNU Lesser General Public License v2.1
3 stars 10 forks source link

[ADC][DataAPI] Discrepency of function usage. #28

Open jalinei opened 5 months ago

jalinei commented 5 months ago

Initialization sequence

  1. spin.adc.configureTriggerSource(ADCx, TRIG)
  2. Optional : spin.adc.configureDiscontinuousMode(x, 0/1)
  3. spin.adc.enableChannel(ADCx, channelx)
  4. Ìf software triggered : data.triggerAcquisition(ADCx)
  5. If hardware triggered : data.start()
  6. Retrieve value : data.getLatest(ADCx, pinx)

The user has to know both pin number and channel number to start a measurment. Either we choose ADC channel of ADC pin but not both !!

jalinei commented 5 months ago

Linked with #13
These two issues are harmful to the user experience. I do suggest to keep the channelx rather than the pinx approach.

Channelx approach is closer to the tag in the pinout image.

cfoucher-laas commented 5 months ago

There seems to be a misunderstanding here as I see you mention "channel number". You're mixing two interfaces: high-level Data API, which uses pins (if you're using Spin alone), and Twist channels names (with Twist enabled). There is no notion of channel number at this level. For more advanced usage, there is the lower-level spin.adc API. This API being low level, it uses the notion of channel number, but is is only intended for use by advanced users.

Your code should be:

spin.adc.configureTriggerSource(ADCx, TRIG) /* Only if not software */
spin.adc.configureDiscontinuousMode(ADCx, 0/1) /* Only if we want discontinuous */
data.enableAcquisition(ADCx, pinx)
If software triggered : data.triggerAcquisition(ADCx)
If hardware triggered : data.start()
Retrieve value : data.getLatest(ADCx, pinx)

Or with Twist:

spin.adc.configureTriggerSource(ADCx, TRIG) /* Only if not software */
spin.adc.configureDiscontinuousMode(ADCx, 0/1) /* Only if we want discontinuous */
data.enableShieldChannel(ADCx, TWIST_CHANNEL_NAME)
If software triggered : data.triggerAcquisition(ADCx)
If hardware triggered : data.start()
Retrieve value : data.getLatest(ADCx, TWIST_CHANNEL_NAME)

As you see here, even while using some of the low-level API, there is no need for knowing the channel number.

jalinei commented 5 months ago

Ok I see.

Can we come up with another name than channel ? Channel is already used by the vendor. It is confusing to reuse it to name measurments for Twist. Can you check in the documentation that there are no mistakes on the snippets and descriptions for both DataAPI and ADC ?

cfoucher-laas commented 5 months ago

You're right about that naming conflict. I used in the API the term we went with for Twist: we should rename this at the Twist level in our wording, and adapt the terminology in the API accordingly.

cfoucher-laas commented 5 months ago

As I see it in the documentation, we should not expose the ADC API first, or at least show a warning at the beginning of the page that this is for advanced used only, and that end-user should only use the Data API (and provide a link to it) for most of the use cases.

jalinei commented 5 months ago

Sure, the nav bar is not definitive yet. My view is to place that API under SPIN API. (by default all APIs have a doxygen generate doc that will be replaced by our written documentation that also include the generated content)

cfoucher-laas commented 5 months ago

I'm working on the dataAPI docs right now. Basically, I integrate information that was already present in the former Wiki, as I already did a big work of documentation there.

jalinei commented 5 months ago

Alright, please make sure that it follows the general templating for APIs.

cfoucher-laas commented 5 months ago

I will, but I think there should be an introduction about what the API is and what it does in plain text before introducing frames containing lists.

cfoucher-laas commented 5 months ago

I corrected the code examples, and added plain-text information : https://owntech-foundation.github.io/Documentation/core/docs/dataAPI/ Please tell me if it's OK or if it beaks the templating.