Maybe two little (long-time) bugs found

This forum is dedicated to software development related to MultiWii.
It is not the right place to submit a setup problem.
Software download
Post Reply
Edgar
Posts: 13
Joined: Tue Jul 17, 2012 2:08 pm

Maybe two little (long-time) bugs found

Post by Edgar »

Hi there!

I'm currently playing around a little bit with the MultiWii v2.2 code, to adapt and extend it to my own needs. Thereby i found two spots in the code, where I think there might be something not correct (maybe it has no practical effect, tough).

But I thought I'd post it here, and maybe some of you devs can have a look at it. I hope, this is welcome.

First spot in "MultiWii.ino", Line #215 in v2.2:

Code: Select all

static uint16_t previousTime = 0;

Shouldn't this variable be declared as "uint32_t", because it's assigned with "previousTime = micros()" later on?

Second spot in "Serial.ino", Line #828 in v2.2:

Code: Select all

return (serialHeadRX[port] - serialTailRX[port])%RX_BUFFER_SIZE;

With this line of code the fill level of the ring buffer in the function "SerialAvailable" is calculated. As I called this function cyclically to display the result as debug-output in the GUI, I noticed, that the result jumps sometimes to a high value - much higher, than the actual buffer size.

I'm currently implementing a non-blocking graupner hott telemetry-feature. I read the telemetry request of the receiver to one of the serial ports. Most of the time I get a "1" or "2" as a result of the "SerialAvailable"-Call, but every once in a while the result jumps to e.g. "194". I have not investigated this further, but it seems that on a special constellation of the Head and Tail variables the calculation fails

this fixes it

Code: Select all

return (serialHeadRX [port] + RX_BUFFER_SIZE - serialTailRX [port]) % RX_BUFFER_SIZE;


These code parts are the same in current pre 2.3 builds, also

I'm curious about your conclusions!
And: I really love the MultiWii-Project - keep up the great work! :-)

Greets,
Edgar

timecop
Posts: 1880
Joined: Fri Sep 02, 2011 4:48 pm

Re: Maybe two little (long-time) bugs found

Post by timecop »

uint32_t is "very slow" on avr.
its usage must be limited t o only when necesasry.
as long as it "sorta works" with uint16, no problem.

User avatar
Hamburger
Posts: 2578
Joined: Tue Mar 01, 2011 2:14 pm
Location: air
Contact:

Re: Maybe two little (long-time) bugs found

Post by Hamburger »

High Edgar
Your findiings are very interesting indeed.
Please continue - and do not gett fooled by some cynical remarks from our long term followers.
Hamburger

Mis
Posts: 203
Joined: Fri Apr 01, 2011 12:23 am

Re: Maybe two little (long-time) bugs found

Post by Mis »

About first bug. The previousTime variable is used only for cycle time calculation, and not have any other purpose. No dangerous problem if is overflowed.

Alexinparis
Posts: 1630
Joined: Wed Jan 19, 2011 9:07 pm

Re: Maybe two little (long-time) bugs found

Post by Alexinparis »

About first oddity:
There is no need to use 4 bytes for previousTime
There is an implicit cast 4->2 bytes here : previousTime = micros();
It's more than enough to store a 65ms delay where no more than 10ms is required.

About second bug, yes it was one ;) and it is corrected (I hope) like this in current dev, doing a cast before modulo:

Code: Select all

  return ((uint8_t)(serialHeadRX[port] - serialTailRX[port]))%RX_BUFFER_SIZE;

Edgar
Posts: 13
Joined: Tue Jul 17, 2012 2:08 pm

Re: Maybe two little (long-time) bugs found

Post by Edgar »

Second issue: Yes, the typecast fixes it! Elegant solution ;-)

First one:
You're right alex, if you say 65ms are far enough to hold the cycletime (variable "cycleTime"). But the point is: "previousTime" holds an absolute timestamp value (controller runtime in micro seconds)

this is how the cycletime is calculated:

Code: Select all

currentTime = micros();
cycleTime = currentTime - previousTime;
previousTime = currentTime;

I've tested it out: I've declared "cycleTime" 32bit, left "previousTime" 16bit and debugged the cycle time - as 32bit. Then the error shows up unmistakeably.

But the funny thing about this is, that it obviously doesn't make a real difference in practise. Because the wrong, much too high result of the subtraction is truncated afterwards

Post Reply