yesterday I found a code that will lock up an arduino mkr zero.
void setup() {
// put your setup code here, to run once:
analogRead(1<<3);
}
void loop() {
// put your main code here, to run repeatedly:
}
I'm pretty sure that the Arduino tries to read a pin that doesn't exist and just freezes up because of it. you can put it into DFU mode to upload stuff to the Arduino, and after you upload a good program the problem goes away.
does anyone know why using bit shift in the analogRead makes the Arduino crash, but just trying to read pin 1000 doesn't?
the valid pin numbers work as expected and trying to read the pins 100 to 262 do nothing, but when you try to read from pin 263 the Arduino freezes.
I did some research, and you need to have an "a" before the pin number to reference an analog pin.
(A0, A1, etc.)
So, using integers in an analog read is definitely going to cause errors.
why are you even allowed to try to read from integers in the first place? trying to analog read from "A7" causes an error but trying to analog read from "7" doesn't.
Ax are #defines; e.g. on an Uno, A0 is replaced by 0, A1 by 1 etc. So they are actually integers. If your board does not have A7, it will not have A7 defined and the compiler will throw an error.
But the compiler expects an integer (or byte, not sure) and any valid integer is acceptable for the compiler.
This is really curious. As @sterretje said, the 'A' pin numbers are just macros, and any number that is not valid should not cause the MCU to hang up.
You are seeing trouble with:
263 dec
0001 0000 0111 bin
and higher.
May need to dig a little more into the function analogRead
EDIT: it expects a uint8_t, so even if it accepts only the last 8 bits, that gives 7 dec (0111 bin).
@cncnotes The reason why the microcontroller crashes, is because the analogRead()'s pin number argument is used to index an array (called g_APinDescription in the board's variant.cpp file). This array defines the properties of each of the board's pins.
However, since the bounds of this array are not checked, (presumably because the SAMD21 core code has to deal with the various boards with varying pin counts), if the index is too large, the code ends up accessing an invalid locations in memory.
Thanks for typing that up.
I looked at wiring-analog in Arduino and couldn't see anything obvious that should cause a hang up.
I believe the MKR zero uses a SAM D21.
Here is an interesting article on setting up the ADC. There are some while statements in there, and I wonder if one of them may be causing the problem.
ok, I did some digging into the source, and I found what is causing the hang, and how to fix it.
the hang happens in the function pinPeripheral in the file wiring_private.c at line 96 or 105 (it changes depending on an if statement) when the function tries to multiplex the pin number.
this can be easily fixed by adding the code below to the analogRead function at line 135 in the file wiring_analog.c
if (g_APinDescription[pin].ulPinType != PIO_ANALOG){
return -1;
}
this will check if the pin is an analog pin before trying to read/multiplex it.
EDIT: I made a github issue, you can find it here if you want
The reason why the MKRZero crashes is because you're entering an incorrect pin number into the analogRead() function. The MKRZero doesn't have an analog pin 8, or 1 << 3.
In your code:
if (g_APinDescription[pin].ulPinType != PIO_ANALOG){
return -1;
}
What happens if the pin number index the exceeds the bounds of the g_APinDescription array, as it does with values like 37 to 52 that you tried above?
With all due respect, you're essentially solving a problem of your own creation.
Excellent work!
Ideally, the compiler should return an error if the pin number is out of bounds, as you note in your code. Certainly, would not want the chip to hang up.
I have a solution too, just don't use a non existent pin!!!
I have not had your problem, and I think it is because I do not try to use pins my controller does not have.. KISS...
@cncnotes On the contrary, it's not excellent. As @TomGeorge says, just don't use a non existent pin.
The compiler won't catch the out of bounds array, because it's a logical error rather than a syntactic one.
@awesome_tornado The analogRead() function by its nature is usually called by the majority of users in the loop() function using a fixed and valid pin number. This means any input checks would be unnecessarily called everytime the function is executed, which is a completely inefficient use of the microcontroller's time.
By all means add input and bounds checks to your own sketch, but please do not impose this unnecessary change on everyone else, by raising it as an issue on the Arduino SAMD21 core code on Github.
there is no out of bounds error that needs to be caught.
the Arduino uno lets you analogRead any pin number without hanging, this hang is not intentional.
a single if statement is nothing, there are other places in the source of the analogRead function that have much more impact on performance. (but this doesn't mean much, because if statements have very little impact.)
I didn't mean to try to use a non existent pin when I first found this problem, I made a typo. and I am sure that I am not the only one that makes typos.
if you dislike this so much, please just make your own code to analogRead. you can make it faster than the built in analogRead function if you want.
this change is not unnecessary, it is meant to prevent typos from making peoples Arduinos hang.
raising something as an issue does not mean that it will be accepted and "imposed" on everyone, and there is no reason for me to not post this as an issue.
Actually, this is an excellent piece of work in finding a bug and reporting it.
Have you tried to see what happens if you use an out of bounds pin number? Please report back after running your own test rather than trying to contradict my opinion with your opinion.
FYI, I have run a similar program on the Uno, and it does not hang up!
And in response to your comment about not using non-existent pins, I highly recommend that every one should code perfectly
In engineering there's often no right or wrong way, just decisions that can be based on acceptable cost/time/speed/accuracy/precision etc...
Personally, I would prefer the efficiency that comes with speed of execution over checking pin validity and bounds when calling analogRead() each time in the loop(), otherwise the microcontroller would be unnecessarily running the same checks over and over again. Programming a small microcontroller isn't the same as programming a PC.
Yes I have, and to be honest I don't have or hold an opinion on the matter. The fact is that compiler compiles the following code for the MKRZero without error:
void setup() {}
void loop() {
int result = analogRead(52); // Read from an invalid pin number
}
Even though 52 is an invalid pin number, the compiler didn't catch the logical error.
Since on the MKR Zero the g_APinDescription array (in board's variant.cpp file) only has 36 elements, this code will attempt to access an invalid index outside the bounds of the g_APinDescription array.
So are you saying that if you enter an invalid analog pin number into analogRead(), it functions correctly?
The solution in this case is simple, if you don't want the microcontroller to crash, don't enter invalid numbers .
Have you actually looked at the source for analogRead? there are checks for validity already present in the function that let you use channel numbers, pin numbers, or built in macros to reference pins. if you are concerned about speed, you should not be using the built in analogRead function, you should be making your own.
Yes indeed. I did exactly that in setup, and then in loop I ran just a simple print statement counting digits. No problems at all with the Uno hanging up.
And I do agree with you about the compiler not being expected to do analysis of the logic of the program.
My point, of course is not that one shouldn't be careful about coding, but that this behaviour seems to be a bug in the Zero.
