ataradov / dgw

USB HID Data Gateway
50 stars 10 forks source link

F_CPU is not defined #2

Open MonteShaffer opened 6 years ago

MonteShaffer commented 6 years ago

Hi,

embedded/debug.c refers to setting up debugger for UART,

line 47 refers to F_CPU which doesn't seem to be defined for the SAMD21.

спасибо

monte

{x:

ataradov commented 6 years ago

F_CPU is defined in the Makefile and the AS project defines configuration.

ryan4volts commented 6 years ago

I know alex doesn't like using the bloated libraries, but I prefer the approach of checking the gclk freq you're using:

const uint32_t clk_freq = system_gclk_gen_get_hz(0); //clk gen 0

ataradov commented 6 years ago

It is not about bloated or not. I KNOW what frequencies are used in my system, since I defined them. I don't need a library to reverse-engineer the value back from my settings.

The next question is why "0"? Would not it be better to make it universal? And that's how you end up with ASF.

Furthermore, this prevents optimization of what is essentially retrieving a constant.

MonteShaffer commented 6 years ago

I concur with Alex that "universal" needs to be appropriate.

Из двух зол выбира́ют ме́ньшее.

Less is more when it is the correct "less." I don't want "universal" to all devices, but "universal access" to all ADCs on this device.

We are playing around with the adc.c code, and believe a little more "abstraction" could be implemented. e.g., ADC defined as PIN(A,3)... why not call it PINA3 as it is only one of 12 potential ADC options on the SAMD21?

We tried this same example on the SAMD11 and hit the 16K ceiling, so we are now using the SAMD21. We are not using Atmel studio anymore, but Keil μvision in our current efforts. It is working as expected, yet we want to possibly make it more abstract.

I am trying to create an abstract function getValueFromADC(A,3) that I may poll often, and I may poll for more than one ADC pins. If successful, your "dgw" project could send multiple-sourced-pin ADC values via USB (as HID), which is our ultimate goal.

Hardcoded MUXEN is also a little unclear. There are 15 options, yet only 9 are defined in hal_gpio.h ... I assume that is the SAMD11 limitation?

... I understand the map of pin and port, but don't see the relationship with the PMUX values ... HAL_GPIO_PMUX_B is assigned with PIN(A,3).

Please clarify: is HAL_GPIO_PMUX_B is from the SAMD21 pinout diagram? Is it specific to ADC (A,3) or for all ADC pins? Is it being used with the "USB jumper"?

I am suggesting that a bit more abstraction may be coded, but I also recognize that "dgw" was not intended to proved full ADC support, just "test it" ... we will work on some ideas and post an update of our work.

Thoughts?

ataradov commented 6 years ago

Because this is not a universal driver. This is just a code for this project and this project is designed for one ADC. I don't treat this file as something I wrote once. If a project comes by that needs more than one ADC channel, I will make necessary changes.

There are not 15 options for PMUX. Actual device only defines values A-H (see Table 7-1). Same table is useful to know why B was selected for the ADC.

To make the ADC function more universal you define and initialize all the pins and then in the adc_read() configure INPUTCTRL to select the requested channel and do the sampling.

ataradov commented 6 years ago

I only code as many abstractions as necessary for a specific project. The amount of code is abysmally minimal that you can't even call it "copy-paste" or whatever everyone is so afraid of.

ataradov commented 6 years ago

So to add multiple channels do something like this:

HAL_GPIO_PIN(ADC0, A, 3) HAL_GPIO_PIN(ADC1, A, 4) HAL_GPIO_PIN(ADC2, A, 5) HAL_GPIO_PIN(ADC3, A, 6)

HAL_GPIO_ADC0_pmuxen(HAL_GPIO_PMUX_B); HAL_GPIO_ADC1_pmuxen(HAL_GPIO_PMUX_B); HAL_GPIO_ADC2_pmuxen(HAL_GPIO_PMUX_B); HAL_GPIO_ADC3_pmuxen(HAL_GPIO_PMUX_B);

static int g_channels[] = {ADC_INPUTCTRL_MUXPOS_PIN1, ADC_INPUTCTRL_MUXPOS_PIN4, ADC_INPUTCTRL_MUXPOS_PIN5, ADC_INPUTCTRL_MUXPOS_PIN6 }; int adc_read(int channel) { ADC->INPUTCTRL.reg = g_channels[channel] | ADC_INPUTCTRL_MUXNEG_GND | ADC_INPUTCTRL_GAIN_DIV2;

ADC->SWTRIG.reg = ADC_SWTRIG_START; while (0 == (ADC->INTFLAG.reg & ADC_INTFLAG_RESRDY));

return ADC->RESULT.reg; }

Here, your function now can read 4 ADC inputs.

You can move "ADC_INPUTCTRL_MUXNEG_GND | ADC_INPUTCTRL_GAIN_DIV2" to the g_channels array, so there is even less code. Plus you can specify static configuration per channel.

MonteShaffer commented 6 years ago

Alex,

Thank you, something similar to the code sample is what we have already developed.

You state There are not 15 options for PMUX. Actual device only defines values A-H (see Table 7-1). Same table is useful to know why B was selected for the ADC.

When we turned on "peripheral debugging" within the KEIL IDE, we saw values changing for "1" and "15" ... you defined HAL_GPIO_PMUX_B as 1. This is why I believe there are 0...15 (or sixteen options). Maybe these are "undocumented"...

ataradov commented 6 years ago

4 bits can hold 16 values, that is true. But that does not mean you should or can use all of them.

Table 7-1 in the datasheet enumerates which ones you can use. The others are not "undocumented", they are not implemented.

MonteShaffer commented 6 years ago

http://ww1.microchip.com/downloads/en/DeviceDoc/40001882A.pdf is the dataseet and http://ww1.microchip.com/downloads/en/DeviceDoc/40001882A.pdf is additional info...

We have SAMD21J ...

ataradov commented 6 years ago

Is there a question? Or why did you post this? It is not that hard to find the datasheets.

MonteShaffer commented 6 years ago

Yes, I agree, but this organizes this thread if people want to refer to Table 7-1.

Cheers,