palmerr23 / Black-F407VET6-cube

Alpha variant for new STM32duino code base https://github.com/stm32duino/Arduino_Core_STM32F4
10 stars 5 forks source link

Some advices #2

Closed fpistm closed 5 years ago

fpistm commented 7 years ago

Hi Richard,

Here some advices about this variant:

In variant.h:

In PeripheralPins.c Avoid to have same pin defined several times for the same feature. Ex in const PinMap PinMap_SPI_MISO[] {PB4, SPI1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)}, {PB4, SPI3, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF6_SPI3)},

BR

palmerr23 commented 7 years ago

Frederic,

Thanks. I'll worl through these over the next few days.

The PeripheralPins adivice was not unexpected. It is unfortunate that this is the case, given the SPI1 and SPI3 pin interchangeability.

Richard

palmerr23 commented 7 years ago

Frederic,

The changes were straightforward and I will upload them once they are tested.

With Analog pins, is repeating the Port enumeration at the end of the digital pin list in variant.cpp the best approach, or should we just rely on the Pin_Ax defines and Ax constants in variant.h?

Richard

fpistm commented 7 years ago

Analog pins must follow as the following functions use an offset:

define analogToPinName(p) ((p < 6) ? digitalToPinName(p+73) : digitalToPinName(p))

// Convert an analog pin number to a digital pin number

define analogToDigital(p) ((p < 6) ? (p+73) : p)

think to update the number of Analog pins declared if it is different of A0-A5 and the offset must be inline with the enum declaration.

palmerr23 commented 7 years ago

Frederic,

Thanks for the further tips.

The obvious thing to do is to create a couple of #defines to take care of the "6" and "73" issues.

The first seems to be taken care of by substituting NUM_ANALOG_INPUTS.

The second would seem to be solved by #define ANALOG_BASE (DEND - NUM_ANALOG_INPUTS)

BTW: With the PinMaps defined in PeripheralPins.c, the value of NUM_ANALOG_INPUTS isn't available to the pre-processor, as sizeof(PinMap_ADC) isn't available to other code until after compile, so using it inline might break the code.

There would seem to be minimal issues with shifting the PinMap definitions to PeripheralPins.h (and moving it into the variant's folder).

Leaving PeripheralPins.c as an empty shell would satisfy Arduino's build rules.

All improvements, except moving definitions from PeripheralPins.c to PeripheralPins.h are now updated in the repo.

Most have only had basic tests.

Richard

fpistm commented 7 years ago

Hi plamer,

I've made an update to those definition:

-//Analog pins
-#define A0                      79
-#define A1                      (A0+1)
-#define A2                      (A0+2)
-#define A3                      (A0+3)
-#define A4                      (A0+4)
-#define A5                      (A0+5)
-
-#define NUM_DIGITAL_PINS        DEND
-#define NUM_ANALOG_INPUTS       (sizeof(PinMap_ADC)/sizeof(PinMap))
-#define MAX_DIGITAL_IOS         NUM_DIGITAL_PINS
-#define MAX_ANALOG_IOS          NUM_ANALOG_INPUTS
+enum {
+  A_START_AFTER = D78,
+  A0,  A1,  A2,  A3,  A4,  A5,
+  AEND
+};
+
+#define MAX_ANALOG_IOS          (sizeof(PinMap_ADC)/sizeof(PinMap))
+#define MAX_DIGITAL_IOS         DEND
+#define NUM_DIGITAL_PINS        MAX_DIGITAL_IOS
+#define NUM_ANALOG_INPUTS       (AEND - A0)
+#define ANALOG_BASE             (DEND - NUM_ANALOG_INPUTS)

 // Convert a digital pin number Dxx to a PinName Pxy
 #define digitalToPinName(p)     ((p < NUM_DIGITAL_PINS) ? digital_arduino[p] : (STM_VALID_PINNAME(p))? (PinName)p : NC)
 // Convert an analog pin number Axx to a PinName Pxy
-#define analogToPinName(p)      ((p < 6) ? digitalToPinName(p+79) : digitalToPinName(p))
+#define analogToPinName(p)      ((p < NUM_ANALOG_INPUTS) ? digitalToPinName(p+ANALOG_BASE) : digitalToPinName(p))
 // Convert an analog pin number to a digital pin number
-#define analogToDigital(p)      ((p < 6) ? (p+79) : p)
+#define analogToDigital(p)      ((p < NUM_ANALOG_INPUTS) ? (p+ANALOG_BASE) : p)

Now, only the enum have to be updated.

palmerr23 commented 7 years ago

Excellent Frederic,

I've updated my code to match the approach you've taken.

I'm starting a new issue for ADC code.

Richard