GreyGnome / EnableInterrupt

New Arduino interrupt library, designed for Arduino Uno/Mega 2560/Leonardo/Due
330 stars 73 forks source link

Error Return for specifying Invalid pin? #16

Closed stickbreaker closed 8 years ago

stickbreaker commented 9 years ago

I looked through the EnableInterrupt.h code, I did not see any error checking for invalid pins. I have used the current version 0.8.1 to implement RTS/CTS handshaking for HardwareSerial::begin(baud,config,CTS_pin,RTS_pin);. I am concerned about passing invalid pin values to EnableInterrupt. I use a FALLING mode interrupt for the CTS_pin. Is there a possible error value I can check to validate the Arduino Pin I passed to your library is valid? How does the library handle a invalid pin? Chuck.

GreyGnome commented 9 years ago

I thought about error handling and decided against it, because I don't know what proper error checking would look like. It may give the programmer a false sense of security because the checking I could do is so limited as to be nearly useless in my opinion.

The only validation we have is to ensure that the pin number given is supported on the current architecture, but it could still be wrong. It's possible to, for example, want pin 9 and give pin 8 by mistake.

I checked WInterrupts.c and there is no error checking. If you don't give the proper interrupt number, the switch statements simply fall off the end and it's a noop.

In the EnableInterrupt library, giving a wrong pin number will give invalid cells to an array, such as digital_pin_to_port_bit_number_PGM[55] on the ATmega328, which would put you deep into some strange memory location. Thus you would get an invalid return from that, and behavior is unknown and undefined (but certainly not good).

Thinking aloud, if I returned -1 on an invalid pin number I'm not sure what I could do in my sketch. There are so few avenues for expressing invalid returns during runtime (except perhaps Serial.print's). It's different than programming on a large system, where you would send an error message to stderr. And on top of everything else you have the memory utilization that error checking entails.

stickbreaker commented 9 years ago

How about a second function bool goodInterruptPin(byte pin); that could be called to validate the pin number. Your Interrupt library simulates all interrupt modes on all interrupt pins already. So all pins support all modes already.

In my application I could embed a Serial.println("Bad CTS PIN") or just go into a while(true); lock. The application would always die during the Serial.begin(baud,config,cts,rts); call in setup().

I do not know the limitation of .h files, but maybe some #ifdef's like the interruptedPin to turn this feature on. Or maybe I could just make a complimentary library that is just for the validation.

I imagined the problem you identified as ATmega328 portPin[55], the unknown, quiet failure would be a *itch to find.

I saw how you are using flags during the #include statements to use the same header file in different ways. Maybe there is some way that the lookup tables could have error ranges embedded. Can a lookup table include negative indexes? portPin[-1] = max value, portPin[-2] = min Value. I come from a Pascal background were the index is not assumed to start at zero.

I'll look at your code to see If I can perceive a way to uses your lookup tables to create a validation routine. Of course, I hope to figure out something that does not duplicate any of your tables(duplication create more opportunities for Errors).

The reason I bring this up, is that I developed my CTS/RTS on a UNO, then moved to a MEGA2560 and used the same pin numbers. D4 was CTS, D2 was RTS, worked find on the UNO, but ....

Chuck.

GreyGnome commented 8 years ago

I need to add ATtiny support (soon!) and Zero support, then move on to another project. I am not personally interested in this functionality. I don't think it should be a core part of the library- too much code added to these little chips- so it would need some #ifdef's to guard it, and turn on if desired- which again makes it of secondary importance and in turn makes my code even more unintelligible than it is today. :-\

My apologies- I'm not against adding it, but I don't want to do it myself.

Regarding the negative indexes- that's not supported in C/C++. C is close to assembler, so the data model is pretty simple; the index is just added to the starting pointer of an array and the information found there is returned (which is why it's easy to march right off the end of an array and get garbage). I suppose a negative index may take you to, for example 255 or something like this.

I'm going to go ahead and close this. If you have anything more to add (code, for example :-), or a swarm of programmers with pitchforks and torches at my door), I can reopen.