Photon and the PIN_MAP[] challenge!

Folks, with the Photon being so new, a lot of new functionality is being introduced to the firmware, including the HAL (Hardware Abstraction Layer) and the modular system/user code. These changes are impacting library code that was adapted specifically for the Core when future products were not known. Well, now that we know that future, it is time to generalize libraries for all Particle devices including the future Electron.

One key function used for doing faster bit I/O uses the PIN_MAP reference. Until @mdma can formalize a method (API, macros) for using PIN_MAP to set and reset pins faster than using (the safer) digitalWrite() command, the following code may be used to implement the functions:

The solution to the PIN_MAP issue is a little more complex due to the differences in the bit set and reset registers on the STM32F103 (core) and the STM32F205 (photon). Here is a set of macros I used to port the SparkIntervalTimer library:

#if defined(PLATFORM_ID)  //Only defined if a Particle device
  #include "application.h"
  STM32_Pin_Info* PIN_MAP = HAL_Pin_Map(); // Pointer required for highest access speed
#if (PLATFORM_ID == 0)  // Core
  #define pinLO(_pin) (PIN_MAP[_pin].gpio_peripheral->BRR = PIN_MAP[_pin].gpio_pin)
  #define pinHI(_pin) (PIN_MAP[_pin].gpio_peripheral->BSRR = PIN_MAP[_pin].gpio_pin)
#elif (PLATFORM_ID == 6) // Photon
  #define pinLO(_pin) (PIN_MAP[_pin].gpio_peripheral->BSRRH = PIN_MAP[_pin].gpio_pin)
  #define pinHI(_pin) (PIN_MAP[_pin].gpio_peripheral->BSRRL = PIN_MAP[_pin].gpio_pin)
#else
  #error "*** PLATFORM_ID not supported by this library. PLATFORM should be Core or Photon ***"
#endif
#endif

You will need to replace any PIN_MAP operation in the code with pinHI and pinLO if you want the code to be common for both the Core and the Photon (and future Electron). :smile:

UPDATE: including pinmap_impl.h is not necessary it seems

12 Likes

With your permission, Iā€™d like to work on this header a little then include it in the firmware, so that we have one copy to maintain, along with any other library compatibility tweaks that people need.

1 Like

@mdma, I was hoping you would say that! :grin:

Excellent!

Ok, so some things we can discuss:

  • use of macros vs inline functions?
  • naming: any suggestions for a name other than pinHI/pinLO?
  • what other things do we need in a library compatibility layer?

Here's my take on these:

  • macros: I'd prefer not to use macros since they can cause very confusing messages if a library also happens to use the same name. (We are gradually phasing out macros in favor of regular C/C++ constructs.) The alternative is to use inline functions. The compiler can optimize these to give as good as performance as macros.
  • naming: How about fastDigitalWrite(pin, value) ?
1 Like

@mdma, I totally agree on inline vs macros so letā€™s use inline. There is some precedence in the Arduino world for creating a ā€œfastDigitalWrite()ā€ function. However, there is more than setting single bits to consider. At the low level we should also have:

fastDigitalRead(pin)
fastPortWrite(port, value)
fastPortRead(port)

:smile:

1 Like

In addition to the digitalFastWrite(pin,value) (and of course @peekay123ā€™s suggestions) where speed is an issue fastPinSet(pin) and fastPinReset(pin) might be useful too.

@ScruffR, good point though I thought those would just be a macro using the fastDigitalWrite() ā€œrootā€ function. :smile:

@mdma, @peekay123, Hi this is great news on for getting this implemented! Do you have any idea of a time line for an updated firmware?

Is there any thing we can do in the mean time to implement this into our existing code?

Thanks!

You could use the existing header file until itā€™s implemented in firmware, and provide a forward compatibility layer, mapping from the proposed function names to the existing ones:

something like:

#define fastDigitalWrite(pin, value)  (value ? pinHI(pin) : pinLO(pin))

A thought on macros vs inline functions:

We should test the speed difference, since this is supposed to be useful for the fastest GPIO writing, if itā€™s notā€¦ then people that need it SUPAFAST will still break out and do things manually.

Make sure people can breakaway and do things manually anyway.

I would prefer the naming be digitalWriteFast() so that when we add intellisense to the IDEā€™s both versions of the function come up intuitively :smile:

2 Likes

In this case the (admittedly minor) impact of the compare instruction would counteract the intended speed gain and with inlining you'd add some additional flash usage for the instruction which will never be called each time you'd use one of these macros (I guess, since I doubt that the optimizer will kill the respective branch).
I'd rather turn that logic round and implement the set/reset functions a priori and use these in a third function or macro depending on the value - as pointed out in @mdma's macro.

@peekay123, I used your macros in two libraries in my code. I now get the following errors.
error: redefinition of 'STM32_Pin_Info* PIN_MAP
error: ā€˜STM32_Pin_Info* PIN_MAPā€™ previously declared here

Not sure what is causing it. I tried reading up on that error but canā€™t figure it out. Any suggestions?

In file included from /spark/compile_service/shared/workspace/6_hal_12_0/firmware-privaili9341_testing.cpp:7:0:
/spark/compile_service/shared/workspace/6_hal_12_0/firmware-privaTouch_4Wire.h:16:18:
error: redefinition of 'STM32_Pin_Info* PIN_MAPā€™
STM32_Pin_Info* PIN_MAP = HAL_Pin_Map(); // Pointer required for highest access speed

In file included from /spark/compile_service/shared/workspace/6_hal_12_0/firmware-privaili9341_testing.cpp:4:0:
/spark/compile_service/shared/workspace/6_hal_12_0/firmware-privaAdafruit_ILI9341.h:27:17:
error: ā€˜STM32_Pin_Info* PIN_MAPā€™ previously declared here
STM32_Pin_Info* PIN_MAP = HAL_Pin_Map(); // Pointer required for highest access speed

Some (most) definitions/declarations are not allowed to be made twice.
For this reason header files usually have some way to prevent this to happen by means of conditional compiling like this

#ifndef _ADAFRUIT_TOUCH4WIRE_H_
#define _ADAFRUIT_TOUCH4WIRE_H_

...

#endif

So you could either remove the second definition/declaration or wrap each of these blocks in a conditional compiling ā€œenvelopeā€ like

#ifndef _PHOTON_SPEED_MACROS_
#define _PHOTON_SPEED_MACROS_
... 
#endif

But unfortunately I have called the respective macros pinSetHigh() and pinSetLow() in my port of Touch_4Wire.
So youā€™d need to adapt @peekay123ā€™s macros or replace all occurences of my macro names with his - sorry :blush:

If your libraries depend on each other, you can put the macros in the lowest level, and just use PIN_MAP, pinHI( ), pinLO( ) in the top level library.

If that doesnā€™t work, just name the pointer PIN_MAP2 in one of the libraries :wink:

@mdma, can you remember this one :wink:

Now we are back in the forum with this topic - even public.
But I'd like to renew my proposal for a porting toolset including most of the collected macros (or even more) - if these should become inline functions, even better :+1:

(Link not publicly available)
http://community.particle.io/t/some-reusable-macros-and-stuff/8846

When the value argument is constant, such as fastDigitalWrite(A0, LOW) then the compiler will indeed optimize out the conditional and simply inline the appropriate branch. Even so, we can implement fastDigitalSet directly if it makes people happy. (We'd probably get others asking the same question about efficiency.)

1 Like

@mdma, @ScruffR, I think I stirred the pot a little to much with this topic! I get the feeling it is causing more problems then not. What I do know is that deploying a solid set of extended functions for fast I/O is crucial to get a lot of libraries ported over to the Photon, especially given folks are receiving their them and have certain ā€œexpectationsā€.

Macros or inline, the timing difference between the ā€œsafeā€ digitalWrite() and the ā€œunsafeā€ digitalWriteFast (or whatever we call it) is huge. Whatever we do, letā€™s get something out that we can test and optimize, as long as the outward facing ā€œfunctionsā€ remain the same. I just want to remind everyone that we need both fast bit and port set/reset capabilities. :grinning:

1 Like

@scruff R
Iā€™m now just trying to get the touch4wire to compile to reduce complexity. When just using this library I get this error: Seems like this is being defined somewhere else not within the libraries?

../../../build/target/user/platform-6/libuser.a(touchscreen.o): In function `setup':
 /spark/compile_service/shared/workspace/6_hal_12_0/firmware-privatouchscreen.cpp:9: multiple definition of `PIN_MAP'
../../../build/target/user/platform-6/libuser.a(Touch_4Wire.o):/spark/compile_service/shared/workspace/6_hal_12_0/firmware-privaTouch_4Wire.cpp:27: first defined here collect2: error: ld returned 1 exit status

Below is the updated macro in the. h file:

#ifndef _ADAFRUIT_TOUCH4WIRE_H_
#define _ADAFRUIT_TOUCH4WIRE_H_

#include "application.h"
#define ADC_MAX_VALUE (0x0FFF)
#define XY_TOLERANCE 15
STM32_Pin_Info* PIN_MAP = HAL_Pin_Map(); // Pointer required for highest access speed
#define pinSetLow(pin) PIN_MAP[pin].gpio_peripheral->BSRRH = PIN_MAP[pin].gpio_pin
#define pinSetHigh(pin) PIN_MAP[pin].gpio_peripheral->BSRRL = PIN_MAP[pin].gpio_pin
#define pinSet(pin, HILO) (HILO) ? pinSetHigh(pin) : pinSetLow(pin)

And here is the .ino file
#include "Touch_4Wire.h"
#define YP A0  // must be an analog pin, use "An" notation!
#define XM A1  // must be an analog pin, use "An" notation!
#define YM D0  // can be a digital pin
#define XP D1  // can be a digital pin

TouchScreen ts = TouchScreen(XP, YP, XM, YM, 300);

void setup(void) {
 // Serial.begin(9600);
}

void loop(void) {
  TSPoint p = ts.getPoint();

  if (p.z > ts.pressureThreshhold) {
     Serial.print("X = "); Serial.print(p.x);
     Serial.print("\tY = "); Serial.print(p.y);
     Serial.print("\tPressure = "); Serial.println(p.z);
  }

  delay(100);
}

Edited by ScruffR: Iā€™ve reformatted your code sections (have a look at ā€œForum Tips and Tricksā€ to see how to do that)

If I comment out the setup function. i get the below error:

Where is this _GLOBAL__sub_I_PIN_MAP function?

./ā€¦/ā€¦/build/target/user/platform-6/libuser.a(touchscreen.o): In function _GLOBAL__sub_I_PIN_MAP': /spark/compile_service/shared/workspace/6_hal_12_0/firmware-privatouchscreen.cpp:13: multiple definition ofPIN_MAPā€™
ā€¦/ā€¦/ā€¦/build/target/user/platform-6/libuser.a(Touch_4Wire.o):/spark/compile_service/shared/workspace/6_hal_12_0/firmware-privaTouch_4Wire.cpp:27:
first defined here

@wesner0019, I had the same problem when trying to port my RGBPongClock code. I ended up putting the macros in the only .cpp file that used PIN_MAP. When I tried putting it in the include file, I got errors even though I had compiles flags setup to only compile that .h file once. Separating the file to a different included file did not help either.

This is why I think it may be worth waiting for the firmware based GPIO stuff that @mdma will be adding (soon).