LeafLabs Garden

A place to share, learn, and grow...

1. 

   gbulmer
   Moderator

   mbolivar

   Out of curiosity, did you also try separate inline and "standard" versions ...

   No. I was experimenting, and I wanted it simple and 'lite', so only one version.
   Also, because my digitalRead and digitalWrite were intended to be simple and very compact, ideally smaller than a call of digitalRead and digitalWrite on int, non-inline versions made no sense to me (IIRC compiling with debug and using GDB works properly)

   ... plus use of __builtin_constant_p() to determine which to use (as tried by Arduino)?

   Definitely not.
   IMHO part of the attraction of the Arduino library is most of it is readable. That looks unnecessarily complex.

   A lot of beginners just declare things like int led = 13; which I think fails the __builtin_constant_p() test.
   So the extra complexity only helps a more experienced developer, but a newbie would have to wade through quite subtle and sophisticated code to understand how it works.
   I'd rather encourage folks to look at the MCU reference manual (e.g. RM0008) than the C++ specification :-)

   My plan was to have a new type which would easily discriminate between the two variations of digitalRead/digitalWrite/pinMode.
   It was also intended to be simpler and more efficient anyway, so faffing around with integer pin numbers was minimised.

   Here's a rough cut early version which I've reconstituted; it didn't even use templates.

   I think digitalRead/digitalWrite are faster and smaller than the standard digitalRead/digitalWrite, but less safe in this version.
   One objective was to make the type no bigger than the default int pin number, so indiscriminate use of Pin is no worse than int.
   

   inline volatile uint32_t* pin_to_bitband(int pin_numb) {  // luxury?
       // should validate pin number
       return bb_perip(PIN_MAP[pin_numb].gpio_device, PIN_MAP[pin_numb].gpio_bit);
   }
   
   class Pin {
   public:
       volatile uint32_t* const bitbandaddress;
       // Construct pin from port address and bit number
       Pin(volatile void* addr, uint8_t bit): bitbandaddress(bb_perip(addr, bit)) {}
   private:
       Pin(): bitbandaddress((volatile uint32_t*)0xFFFFFFFF) {} // cheap and nasty: force an exception
   };
   
   inline void digitalWrite(Pin p, uint32_t value) { *(p.bitbandaddress) = value; }
   inline uint32_t digitalRead(Pin p) { return *(p.bitbandaddress+MAGIC_CONSTANT); }
   
   void pinMode(Pin p, WiringPinMode mode) {
       /* quick and dirty: find pin in PIN_MAP, then call pinMode(int,...) */
       for (int i=0; i<BOARD_NR_GPIO_PINS; i++) {
           if (p.bitbandaddress == pin_to_bitband(i)) {
               pinMode(i, mode);
               break;
           }
       }
       /* proper: use the bitband address to derive control register addresses */
   }
   
   const Pin D0(GPIOA, 3);
   // ...
   const Pin D5(GPIOB, 6);
   // ...
   const Pin D13(GPIOA, 5);
   
   // -- blink --
   
   Pin led = D13;
   
   void setup() {
       pinMode(led, OUTPUT);
   }
   
   void loop() {
       digitalWrite(led, HIGH);
       delay(1000);
       digitalWrite(led, LOW);
       delay(1000);
   }

   It doesn't do pinMode 'properly'.
   It searches for a matching bitband address in PIN_MAP, and used the existing pinMode.
   It should calculate the control registers from the bit band address, and hence be more direct, and not dependant on PIN_MAP.

   Edit {
   I think an inline Pin digitalWrite or digitalRead could be the same number of instructions as the call of the standard int digitalWrite or digitalRead, and hence always smaller because the body of a 'standard' digitalWrite and digitalRead isn't needed. I also think a sequence of inline-calls might be fewer instructions than a sequence of standard calls because the compiler could optimise better.

   Hopefully we can agree that:
   

   inline void digitalWrite(Pin p, uint32_t value) { *(p.bitbandaddress) = value; }
   inline uint32_t digitalRead(Pin p) { return *(p.bitbandaddress+MAGIC_CONSTANT); }

   isn't too sophisticated C/C++ ?
   So someone trying to understand this is best directed to ARM Cortex-M3 documentation, and the MCU Reference Manual :-)
   } End-Edit

   The next step was to use templating so that the Pin instances were proper compile time constants.

   That might have made the inline digitalRead and digitalWrite smaller and quicker by removing the load-bitband-address-from-RAM, because the bitband address would be a comple-time constant.

   I didn't complete that templated variation because, at the time, some of the error messages I accidentally triggered were so opaque I felt people would be left flummoxed; the error messages mentioned templates, which I felt was too off-putting for non-C++ programmers. Maybe I should have tried more variations to try to make the errors easier?

   Edit:
   Trimmed the Pin class to bare essentials; I hope it's clearer.
   Also, I realise e.g.
   inline void digitalWrite(Pin p, uint32_t value) ... etc. could be
   inline void digitalWrite(Pin& p, uint32_t value), but I didn't do that experiment.
   I don't think C programmers would normally use references, so this way is a useful test.

   (WARNING: I may have made errors reconstituting this)

   Posted 3 years ago #
2. 

   mbolivar
   LeafLabs

   Interesting.

   One concern that you may have already considered: what happens when people try to treat a Pin as if it were a number?

   Sounds like to get that to work properly, we'd need to override the arithmetic operators to look up the pin's number via PIN_MAP and do the right thing with it, with the attendant error message concerns. The same thing would apply to comparisons (I can see someone typing pin <= BOARD_NR_GPIO_PINS, e.g.).

   Posted 3 years ago #
3. 

   gbulmer
   Moderator

   mbolivar

   One concern that you may have already considered: what happens when people try to treat a Pin as if it were a number?

   That seems quite exotic to me, but show me the example or use case where people do that now, and I might be able to propose a solution.

   I think I did have a function which mapped the bit band address to a pin number. It was used in pinMode instead of the for loop. But I thought it was muddying the simplicity of the approach, and this way pinMode is very simple and somewhat 'fail-safe'.

   Sounds like to get that to work properly, we'd need to override the arithmetic operators to look up the pin's number via PIN_MAP and do the right thing with it

   That seems like a pile of work, and maybe premature optimisation.
   Show the example and use cases, and we might come up with good proposals. My 'knee-jerk' reaction, without understanding enough about the actual use case, is to provide a way to map bitband addresses to useful numbers, and maybe an appropriate constant.

   The same thing would apply to comparisons (I can see someone typing pin <= BOARD_NR_GPIO_PINS, e.g.).

   Why? I can imagine == and !=, but what's the example or use case for "pin <= BOARD_NR_GPIO_PINS"?

   Posted 3 years ago #
4. 

   mbolivar
   LeafLabs

   Why? I can imagine == and !=, but what's the example or use case for "pin <= BOARD_NR_GPIO_PINS"?

   well, here's pin >= BOARD_NR_GPIO_PINS: https://github.com/leaflabs/libmaple/blob/9a081c2d523252370ebcd100de1f3c57759ebcf5/wirish/wirish_digital.cpp#L49

   Posted 3 years ago #
5. 

   gbulmer
   Moderator

   mbolivar - (you were up late! I hope you're well.)

   Okay so this is validating the pin number:
   

   void digitalWrite(uint8 pin, uint8 val) {
       if (pin >= BOARD_NR_GPIO_PINS) {
           return;
       }
   
       gpio_write_bit(PIN_MAP[pin].gpio_device, PIN_MAP[pin].gpio_bit, val);
   }

   It silently does nothing when the pin number isn't valid.

   So, I think I understand that example, but I think the use case for my experimental class was different; I had hoped to eventually identify errors at compile time.

   My aim was to check validity of all Pin instances at construction.
   The overriding goal of the experiments were to keep digitalWrite and digitalRead minimal so that inline would be comparable to a function call in size, and as quick as practical.

   Also, I was assuming that if there was a good template-based solution, the error would be raised at compile time.
   The approach for this experimental class was weaker.

   So something like this (assuming the LeafLabs error handler which throbs led13 for assertion failure):
   

   //...
   #define GPIOA_VALID_PINS (0x0000E7FF)         // valid GPIOA pins for Maple
   #define GPIOB_VALID_PINS (0x0000FFFB)         // valid GPIOB pins for Maple
   #define GPIOC_VALID_PINS (0x0000E7FF)         // valid GPIOC pins for Maple
   #define GPIOD_VALID_PINS (0x00000004)         // valid GPIOD pins for Maple
   #define ASSERT_VALID_PIN(t) if (!(t)) pin_assert_failure("Pin::Pin failure", __LINE__)
   
   // ...
   
   class Pin {
   public:
       volatile uint32_t* const bitbandaddress;
       Pin(volatile void* addr, uint8_t bit): bitbandaddress(bb_perip(addr, bit)) {
           if (addr == GPIOA) {
               ASSERT_VALID_PIN((0x1<<bit) & GPIOA_VALID_PINS);
           } else if (addr == GPIOB) {
               ASSERT_VALID_PIN((0x1<<bit) & GPIOB_VALID_PINS);
           } else if (addr == GPIOC) {
               ASSERT_VALID_PIN((0x1<<bit) & GPIOC_VALID_PINS);
           } else if (addr == GPIOD) {
               ASSERT_VALID_PIN((0x1<<bit) & GPIOC_VALID_PINS);
           } else {
               ASSERT_VALID_PIN(false);
           }
       }
   
   private:
       Pin(): bitbandaddress((volatile uint32_t*)0xFFFFFFFF) {} // cheap and nasty: force an exception
   };

   In retrospect, a solution more like Maples digitalWrite might have been:
   

   inline volatile uint32_t* safebb(volatile void* addr, uint8_t bit) {
       if (addr == GPIOA && ((0x1<<bit) & GPIOA_VALID_PINS)) {
           return bb_perip(addr, bit);
       } else if (addr == GPIOB && ((0x1<<bit) & GPIOB_VALID_PINS)) {
           return bb_perip(addr, bit);
       } else if (addr == GPIOC && ((0x1<<bit) & GPIOC_VALID_PINS)) {
           return bb_perip(addr, bit);
       } else if (addr == GPIOD && ((0x1<<bit) & GPIOC_VALID_PINS)) {
           return bb_perip(addr, bit);
       }
       return bb_perip(GPIOD, 0);  // harmless pin
   }
   // ...
   Pin(volatile void* addr, uint8_t bit): bitbandaddress(safebb(addr, bit)) {}

   So, at construction, the pin is either initialised correctly, or it is set to a harmless digital I/O address, e.g. bb_perip(GPIOD, 0);. That bitband address would be whatever digital I/O address would work silently in my digitalWrite and digitalRead. That would give some of the behaviour of Maple's digitalWrite.
   I prefer the 'proper' error in my old code :-)

   Posted 3 years ago #
6. 

   modkit
   Member

   Thanks @siy, @gbulmer, and @mbolivar for a wonderful discussion! Sorry we're coming to this late but we hope to continue this focus on "pins as objects" over here

   Posted 3 years ago #

RSS feed for this topic

Reply

You must log in to post.