New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NUM_DIGITAL_PINS and NUM_ANALOG_INPUTS breaks cpp conditionals #140

Closed
bperrybap opened this Issue Oct 25, 2017 · 15 comments

Comments

Projects
None yet
3 participants
@bperrybap

bperrybap commented Oct 25, 2017

The definitions of NUM_DIGITAL_PINS and NUM_ANALOG_INPUTS below:

#define NUM_DIGITAL_PINS            ((uint32_t)DEND)
#define NUM_ANALOG_INPUTS ((uint32_t)(AEND-A0))

breaks code that use cpp conditionals like this:
#if NUM_ANALOG_INPUTS > 4

I've not run into this issue in any other core.

The cpp processor is not type aware and so the uint32_t cast is what breaks things.
When the cast is removed, the cpp conditionals that use the macros will work.

I'm not sure why the cast would be necessary in the macros.
enums can normally be used as integers, and the NUM_ANALOG_INPUTS is already depending on that in the calculation.

If there are some places that get warnings because of some sort of int vs unsigned int comparisons, it seems like it should be fixed there vs in the actual macro definitions.

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Oct 25, 2017

Member

Right it was added to avoid warning.
So, I will remove this cast here and correct all warning raised.

Member

fpistm commented Oct 25, 2017

Right it was added to avoid warning.
So, I will remove this cast here and correct all warning raised.

@fpistm fpistm self-assigned this Oct 25, 2017

@fpistm fpistm added the Bug label Oct 25, 2017

@fpistm fpistm added this to the Next release milestone Oct 25, 2017

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Oct 26, 2017

Member

Hi @bperrybap,
in fact use the NUM_DIGITAL_PINS and NUM_ANALOG_INPUTS is not possible for conditional preprocessing as they are enum values (A0, AEND, PEND).
Instead of use:
#if NUM_ANALOG_INPUTS > 4
you will have to use standard comparison:
if ( NUM_ANALOG_INPUTS > 4)
I could remove the cast but the #if NUM_ANALOG_INPUTS > 4 will be never evaluated true

Member

fpistm commented Oct 26, 2017

Hi @bperrybap,
in fact use the NUM_DIGITAL_PINS and NUM_ANALOG_INPUTS is not possible for conditional preprocessing as they are enum values (A0, AEND, PEND).
Instead of use:
#if NUM_ANALOG_INPUTS > 4
you will have to use standard comparison:
if ( NUM_ANALOG_INPUTS > 4)
I could remove the cast but the #if NUM_ANALOG_INPUTS > 4 will be never evaluated true

@bperrybap

This comment has been minimized.

Show comment
Hide comment
@bperrybap

bperrybap Oct 26, 2017

Bummer. when I did my simple test, I only tested that it compiled not that it worked.

The issue in my case is that I need a test that works at compile time.
My code is looking to see if symbols A4 and A5 exist in the core and it uses the NUM_ANALOG_INPUTS to detect this. If the check is moved to run time, then while it would work for the STM core it, or any core that has the symbols, would fail on any core that didn't define the symbols A4 and A5 since A4 and A5 would always be referenced.
And unfortunately those symbols are sometimes defines and sometimes const ints so there is no way to use the A4/A5 symbols themselves as conditionals to detect it.

I'm not sure how much other code out there does this type of compile time checking using these symbols.

Its a tough problem for something so simple.

I guess I could switch to using the newer variant PIN_XXX defines which includes the PIN_An defines for the analog pins but the STM core currently does not have any of those defines yet nor does many other cores including the older arduino.cc IDE avr core.

bperrybap commented Oct 26, 2017

Bummer. when I did my simple test, I only tested that it compiled not that it worked.

The issue in my case is that I need a test that works at compile time.
My code is looking to see if symbols A4 and A5 exist in the core and it uses the NUM_ANALOG_INPUTS to detect this. If the check is moved to run time, then while it would work for the STM core it, or any core that has the symbols, would fail on any core that didn't define the symbols A4 and A5 since A4 and A5 would always be referenced.
And unfortunately those symbols are sometimes defines and sometimes const ints so there is no way to use the A4/A5 symbols themselves as conditionals to detect it.

I'm not sure how much other code out there does this type of compile time checking using these symbols.

Its a tough problem for something so simple.

I guess I could switch to using the newer variant PIN_XXX defines which includes the PIN_An defines for the analog pins but the STM core currently does not have any of those defines yet nor does many other cores including the older arduino.cc IDE avr core.

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Oct 26, 2017

Member

Could you provide your code? Or at least tell me which operation you performed on the Ax?

If only analogRead, why not iterate with index?

  for (int i=0; i<NUM_ANALOG_INPUTS; i++)
  {
    // read the analog in value:
    sensorValue = analogRead(i);
  
    // print the results to the serial monitor:
    Serial.print("A");
    Serial.print(i);
    Serial.print(": ");
    Serial.print(sensorValue);
    Serial.print("\t");
  }

edit: Issue closed by error.

Member

fpistm commented Oct 26, 2017

Could you provide your code? Or at least tell me which operation you performed on the Ax?

If only analogRead, why not iterate with index?

  for (int i=0; i<NUM_ANALOG_INPUTS; i++)
  {
    // read the analog in value:
    sensorValue = analogRead(i);
  
    // print the results to the serial monitor:
    Serial.print("A");
    Serial.print(i);
    Serial.print(": ");
    Serial.print(sensorValue);
    Serial.print("\t");
  }

edit: Issue closed by error.

@fpistm fpistm closed this Oct 26, 2017

@fpistm fpistm reopened this Oct 26, 2017

@bperrybap

This comment has been minimized.

Show comment
Hide comment
@bperrybap

bperrybap Oct 26, 2017

It is in a diagnostic sketch that comes with one of my libraries.

#if NUM_ANALOG_INPUTS > 5
	Serial.print(F(" A4: digital pin: "));
	Serial.println(A4);
	Serial.print(F(" A5: digital pin: "));
	Serial.println(A5);
#endif

It is showing the digital pin mappings for the analog pins, if they exist.
I can probably work around it using the PIN_Ax defines since this is not really critical code but simply diagnostic output. Unfortunately that will limit the versions of the IDE and the cores that it will work on since the PIN_XXX defines are relatively new.

For the most part users of this diagnostic tool wouldn't miss the messages if they didn’t' work, and the messages were more for me to help them when they had issues.

bperrybap commented Oct 26, 2017

It is in a diagnostic sketch that comes with one of my libraries.

#if NUM_ANALOG_INPUTS > 5
	Serial.print(F(" A4: digital pin: "));
	Serial.println(A4);
	Serial.print(F(" A5: digital pin: "));
	Serial.println(A5);
#endif

It is showing the digital pin mappings for the analog pins, if they exist.
I can probably work around it using the PIN_Ax defines since this is not really critical code but simply diagnostic output. Unfortunately that will limit the versions of the IDE and the cores that it will work on since the PIN_XXX defines are relatively new.

For the most part users of this diagnostic tool wouldn't miss the messages if they didn’t' work, and the messages were more for me to help them when they had issues.

@bperrybap

This comment has been minimized.

Show comment
Hide comment
@bperrybap

bperrybap Oct 26, 2017

You may want to consider what it takes to implement all the new variant PIN_XXX defines as it is likely intertwined with this.
Not sure how to best automate generating the PIN_An defines when using an enum for the An values.
You might be able to do the reverse from what is in the IDE AVR bundled variants and define the PIN_An macros based on the An values.
i.e.
#define PIN_A0 A0
etc...
However, you can't use conditionals to automatically generate only the ones that exist in the enum.

But you might be able to automate filling in the enum and generating the PIN_An defines if you define NUM_ANALOG_INPUTS as a simple number and conditionally used that to populate the enum and to generate the PIN_An defines.
It would be multiple #if lines (a #if for each potential valid entry) and you might be able to move some of this off into a common header so the variant files only define the number of inputs and perhaps the base digital pin number.

bperrybap commented Oct 26, 2017

You may want to consider what it takes to implement all the new variant PIN_XXX defines as it is likely intertwined with this.
Not sure how to best automate generating the PIN_An defines when using an enum for the An values.
You might be able to do the reverse from what is in the IDE AVR bundled variants and define the PIN_An macros based on the An values.
i.e.
#define PIN_A0 A0
etc...
However, you can't use conditionals to automatically generate only the ones that exist in the enum.

But you might be able to automate filling in the enum and generating the PIN_An defines if you define NUM_ANALOG_INPUTS as a simple number and conditionally used that to populate the enum and to generate the PIN_An defines.
It would be multiple #if lines (a #if for each potential valid entry) and you might be able to move some of this off into a common header so the variant files only define the number of inputs and perhaps the base digital pin number.

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Oct 27, 2017

Member

What's you proposed is to define NUM_ANALOG_INPUTS in each variant.h file but it will bring some risk to have misalignment.
Also defining PIN_Ax is a duplicated definition.

You could be simply replace the code by a loop using the standard macro analogInputToDigitalPin :

  for (int i=0; i<NUM_ANALOG_INPUTS; i++)
  {
    // print the results to the serial monitor:
    Serial.print(F(" A"));
    Serial.print(i);
    Serial.print(F(" : digital pin: "));
    Serial.println(analogInputToDigitalPin(i));
  }

Moreover some STM32 could have up to 18 Ax definition.

Member

fpistm commented Oct 27, 2017

What's you proposed is to define NUM_ANALOG_INPUTS in each variant.h file but it will bring some risk to have misalignment.
Also defining PIN_Ax is a duplicated definition.

You could be simply replace the code by a loop using the standard macro analogInputToDigitalPin :

  for (int i=0; i<NUM_ANALOG_INPUTS; i++)
  {
    // print the results to the serial monitor:
    Serial.print(F(" A"));
    Serial.print(i);
    Serial.print(F(" : digital pin: "));
    Serial.println(analogInputToDigitalPin(i));
  }

Moreover some STM32 could have up to 18 Ax definition.

@bperrybap

This comment has been minimized.

Show comment
Hide comment
@bperrybap

bperrybap Oct 27, 2017

I'm not sure how PIN_Ax is a duplicate definition - yes it is a an additional definition over An but that is the direction arduino.cc has chosen to go.
They (arduino.cc in their variants) added PIN_Ax symbols as defines where as An symbols are static const uint8_t (the An symbols moved away from defines many years ago)
The PIN_XXX symbols are all defines. This was their decision from years of people wanting ways to compile time detect if certain pins existed. SPI pins, I2C pins, analog pins etc...
Code can now use the PIN_XXX symbols with cpp conditionals to detect if these pins exist.
And for backward compatibility they left the old/existing symbols which includes the An symbols.
Sad part is that Paul Stoffregen proposed having a core pins header file with some clever macros nearly 10 years ago, long before the 1.0 release that solved all these issues and provided an easy way to map to raw i/o port pins on the AVR parts without having to do runtime table lookups.
The arduino team back then refused.
But recently they came back to this PIN_XXX solution which is isn't as good as Paul's core pins and the associated macros - which he still uses today and is why his AVR digitalpin core functions are 40x faster than the arduino IDE bundled AVR core functions.

In my case I only want to print those two pins and only in certain circumstances.
It relates to I2C and how the pins are mapped so that loop of printing all the pins is not useful.

There should not be a risk of having misalignment, not when using the conditionals I talked about.

In your variants, it appears that the analog pin numbers all use contiguous
incrementing numbers as well as are on contiguous incrementing digital pins.
That is nice and makes things simple.
(This is not always the case like with the variants in the AVR 1284 core)

Here is an example of what I was mentioning.
In variant.h file it is very simple:

// define the number of analog inputs and the digital pin where they start
#define NUM_ANALOG_INPUTS 6
#define A_START_AFTER 15

In a common file like pins_arduino.h

enum {
#if NUM_ANALOG_INPUTS > 0
        A0 = A_START_AFTER + 1,
#endif
#if NUM_ANALOG_INPUTS > 1
        A1,
#endif
#if NUM_ANALOG_INPUTS > 2
        A2,
#endif

        * * * (as many as you might need)

#if NUM_ANALOG_INPUTS > 18
        A18,
#endif
        AEND
};
// but since NUM_ANALOG_INPUTS is directly defined,
// you won't need AEND anymore. (maybe just as a placeholder to deal with the commas above)

// And then the PIN_An defines
#if NUM_ANALOG_INPUTS > 0
#define PIN_A0 A0
#endif
#if NUM_ANALOG_INPUTS > 1
#define PIN_A1 A1
#endif
***
#if NUM_ANALOG_INPUTS > 18
#define PIN_A18 A18
#endif

you could do also do it this way in pins_arduino. h to be more compatible with the newer arduino.cc variant
core pin declarations

#if NUM_ANALOG_INPUTS > 0
#define PIN_A0 (A_START_AFTER + 1)
#endif
#if NUM_ANALOG_INPUTS > 1)
#define PIN_A1 (A0 +1)
#endif
***
#if NUM_ANALOG_INPUTS > 18)
#define PIN_A18 (A17 +1)
#endif

#if defined(PIN_A0)
static const uint8_t A0 = PIN_A0;
#endif
#if defined(PIN_A2)
static const uint8_t A2 = PIN_A2;
#endif
***
#if defined(PIN_A18)
static const uint8_t A18 = PIN_A18;
#endif

Like I said not too pretty, but it keeps the variant files simple/clean and the messy stuff is all in the one common file like pins_arduino.h

bperrybap commented Oct 27, 2017

I'm not sure how PIN_Ax is a duplicate definition - yes it is a an additional definition over An but that is the direction arduino.cc has chosen to go.
They (arduino.cc in their variants) added PIN_Ax symbols as defines where as An symbols are static const uint8_t (the An symbols moved away from defines many years ago)
The PIN_XXX symbols are all defines. This was their decision from years of people wanting ways to compile time detect if certain pins existed. SPI pins, I2C pins, analog pins etc...
Code can now use the PIN_XXX symbols with cpp conditionals to detect if these pins exist.
And for backward compatibility they left the old/existing symbols which includes the An symbols.
Sad part is that Paul Stoffregen proposed having a core pins header file with some clever macros nearly 10 years ago, long before the 1.0 release that solved all these issues and provided an easy way to map to raw i/o port pins on the AVR parts without having to do runtime table lookups.
The arduino team back then refused.
But recently they came back to this PIN_XXX solution which is isn't as good as Paul's core pins and the associated macros - which he still uses today and is why his AVR digitalpin core functions are 40x faster than the arduino IDE bundled AVR core functions.

In my case I only want to print those two pins and only in certain circumstances.
It relates to I2C and how the pins are mapped so that loop of printing all the pins is not useful.

There should not be a risk of having misalignment, not when using the conditionals I talked about.

In your variants, it appears that the analog pin numbers all use contiguous
incrementing numbers as well as are on contiguous incrementing digital pins.
That is nice and makes things simple.
(This is not always the case like with the variants in the AVR 1284 core)

Here is an example of what I was mentioning.
In variant.h file it is very simple:

// define the number of analog inputs and the digital pin where they start
#define NUM_ANALOG_INPUTS 6
#define A_START_AFTER 15

In a common file like pins_arduino.h

enum {
#if NUM_ANALOG_INPUTS > 0
        A0 = A_START_AFTER + 1,
#endif
#if NUM_ANALOG_INPUTS > 1
        A1,
#endif
#if NUM_ANALOG_INPUTS > 2
        A2,
#endif

        * * * (as many as you might need)

#if NUM_ANALOG_INPUTS > 18
        A18,
#endif
        AEND
};
// but since NUM_ANALOG_INPUTS is directly defined,
// you won't need AEND anymore. (maybe just as a placeholder to deal with the commas above)

// And then the PIN_An defines
#if NUM_ANALOG_INPUTS > 0
#define PIN_A0 A0
#endif
#if NUM_ANALOG_INPUTS > 1
#define PIN_A1 A1
#endif
***
#if NUM_ANALOG_INPUTS > 18
#define PIN_A18 A18
#endif

you could do also do it this way in pins_arduino. h to be more compatible with the newer arduino.cc variant
core pin declarations

#if NUM_ANALOG_INPUTS > 0
#define PIN_A0 (A_START_AFTER + 1)
#endif
#if NUM_ANALOG_INPUTS > 1)
#define PIN_A1 (A0 +1)
#endif
***
#if NUM_ANALOG_INPUTS > 18)
#define PIN_A18 (A17 +1)
#endif

#if defined(PIN_A0)
static const uint8_t A0 = PIN_A0;
#endif
#if defined(PIN_A2)
static const uint8_t A2 = PIN_A2;
#endif
***
#if defined(PIN_A18)
static const uint8_t A18 = PIN_A18;
#endif

Like I said not too pretty, but it keeps the variant files simple/clean and the messy stuff is all in the one common file like pins_arduino.h

@bperrybap

This comment has been minimized.

Show comment
Hide comment
@bperrybap

bperrybap Oct 27, 2017

I corrected the examples on github, so make sure to look on github rather than the email.

bperrybap commented Oct 27, 2017

I corrected the examples on github, so make sure to look on github rather than the email.

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Oct 27, 2017

Member

Ok. I will check that in a week. I'm on vacation this week.

Member

fpistm commented Oct 27, 2017

Ok. I will check that in a week. I'm on vacation this week.

@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Nov 6, 2017

Member

I've created a dedicated post on stm32duino.com forum:
http://stm32duino.com/viewtopic.php?f=49&t=2779

Member

fpistm commented Nov 6, 2017

I've created a dedicated post on stm32duino.com forum:
http://stm32duino.com/viewtopic.php?f=49&t=2779

@lacklustrlabs

This comment has been minimized.

Show comment
Hide comment
@lacklustrlabs

lacklustrlabs Jan 15, 2018

Contributor

Another approach could be to keep the AEND and DEND as-is.
Add hardcoded #defines in variant.h (remove them from pins_arduino_var.h)

#define NUM_DIGITAL_PINS    22 // or whatever
#define NUM_ANALOG_INPUTS 5 // just an example

Then add a static_assert somewhere that checks that there are no mismatch

static_assert(NUM_DIGITAL_PINS==((uint32_t)DEND))
static_assert(NUM_ANALOG_INPUTS==((uint32_t)(AEND-A0)))
Contributor

lacklustrlabs commented Jan 15, 2018

Another approach could be to keep the AEND and DEND as-is.
Add hardcoded #defines in variant.h (remove them from pins_arduino_var.h)

#define NUM_DIGITAL_PINS    22 // or whatever
#define NUM_ANALOG_INPUTS 5 // just an example

Then add a static_assert somewhere that checks that there are no mismatch

static_assert(NUM_DIGITAL_PINS==((uint32_t)DEND))
static_assert(NUM_ANALOG_INPUTS==((uint32_t)(AEND-A0)))
@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Jan 27, 2018

Member

First part merged, thanks @lacklustrlabs. (#206)
I will submit the last part this WE.

Member

fpistm commented Jan 27, 2018

First part merged, thanks @lacklustrlabs. (#206)
I will submit the last part this WE.

fpistm added a commit to fpistm/Arduino_Core_STM32 that referenced this issue Jan 27, 2018

[Arduino compatibility] Ax pin definition
Ax definition is now inline with Arduino style
Variant has only to define the digital pin number
of the first analog input  (i.e. which digital pin is A0)

Clean variant header include

Fix stm32duino#140

Signed-off-by: Frederic Pillon <frederic.pillon@st.com>
@fpistm

This comment has been minimized.

Show comment
Hide comment
@fpistm

fpistm Jan 27, 2018

Member

@bperrybap @lacklustrlabs
I've pushed the PR #208 , could you check if it's fine for you.
Thanks in advance

Member

fpistm commented Jan 27, 2018

@bperrybap @lacklustrlabs
I've pushed the PR #208 , could you check if it's fine for you.
Thanks in advance

@fpistm fpistm added the on going label Jan 27, 2018

fpistm added a commit to fpistm/Arduino_Core_STM32 that referenced this issue Jan 27, 2018

[Arduino compatibility] Ax pin definition
Ax definition is now inline with Arduino style
Variant has only to define the digital pin number
of the first analog input  (i.e. which digital pin is A0)

Clean variant header include

Fix stm32duino#140

Signed-off-by: Frederic Pillon <frederic.pillon@st.com>
@bperrybap

This comment has been minimized.

Show comment
Hide comment
@bperrybap

bperrybap Jan 27, 2018

I checked out and used the branch fpistm:arduino_compatibility_pin_140 and I can now compile my test code and lcd library. I don't have any actual STM32 h/w to test it on yet, but the compile seems to be working as expected when using conditional compilation that use the macros.

The only issues were some unrelated warnings in the hal code which vary by board.

But for the pins_arduino.h / variant macros, it looks good.

BTW, you can test the NUM_ANALOG_INPUTS define by using my hd44780 library (install using the IDE library manager) and then compile the sketch I2CexpDiag in the hd44780_I2Cexp i/o class
examples: hd44780->ioClass->hd44780_I2Cexp->I2CexpDiag

bperrybap commented Jan 27, 2018

I checked out and used the branch fpistm:arduino_compatibility_pin_140 and I can now compile my test code and lcd library. I don't have any actual STM32 h/w to test it on yet, but the compile seems to be working as expected when using conditional compilation that use the macros.

The only issues were some unrelated warnings in the hal code which vary by board.

But for the pins_arduino.h / variant macros, it looks good.

BTW, you can test the NUM_ANALOG_INPUTS define by using my hd44780 library (install using the IDE library manager) and then compile the sketch I2CexpDiag in the hd44780_I2Cexp i/o class
examples: hd44780->ioClass->hd44780_I2Cexp->I2CexpDiag

fpistm added a commit to fpistm/Arduino_Core_STM32 that referenced this issue Jan 29, 2018

[Arduino compatibility] Ax pin definition
Ax definition is now inline with Arduino style
Variant has only to define the digital pin number
of the first analog input  (i.e. which digital pin is A0)

Clean variant header include

Fix stm32duino#140

Signed-off-by: Frederic Pillon <frederic.pillon@st.com>

@fpistm fpistm closed this in #208 Jan 29, 2018

@fpistm fpistm removed the on going label Mar 15, 2018

@fpistm fpistm modified the milestones: Next release, 1.2.0 Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment