Critique this code

[Ben Englert]

Hey guys,

Since it's now "off season" for my team (no Championship for us this year), I've been working on some code just to keep myself busy...

In this code I've brought together a few concepts that I've either come up with or seen implemented elsewhere. The code successfully:
1) Offers an interactive menu system over the serial port for tweaking of PID constants
2) Stores Kp, Ki, and Kd values in the EEPROM, populates the RAM with them at boot time, and updates the RAM every time someone makes a change to the constants through the menu system
3) Scales joystick inputs in terms of percentages - 1072 is considering building a mecanum drive as an off-season project, so this kind of abstraction is key because of the math involved in calculating mecanum wheel vectors and speeds
4) Hardware abstraction - using #defines to state where things are hooked up (in hal.h) because electrical people like to change things at the last minute, and it makes everything more readable.

Anyway, please have a look at it here (http://galactica.miazmatic.com/~ben/robotics/code-devel/) and leave any commentary or suggetsions you might have.

Edit: In an effort to uphold both IFI's and Kevin Watson's wishes as far as what may be done with their code, I have only uploaded things that I wrote from scratch. Kevin's frc_encoders, frc_serial_ports, and frc_eeprom code are used, and the three handler routines (UI_Handler, Storage_Handler, Control_System_Handler) are called from IFI's process_data_from_master_uP.

[Matt Krass]

I cannot connect to the server you hosted your code on, I don't know if it's my end or yours, but any suggestions?

[Jeff Rodriguez]

I cannot connect to the server you hosted your code on, I don't know if it's my end or yours, but any suggestions?

I can't get there just fine. Must be on your end.

[Matt Krass]

Definitely odd, is it a new server perhaps? Maybe my DNS hasn't updated, could somebody resolve the IP and send it to me?

[sur2124]

I could get there just fine, the code is a little unorganized, can you formatt it(it would help alot).

[Ryan M.]

Definitely odd, is it a new server perhaps? Maybe my DNS hasn't updated, could somebody resolve the IP and send it to me?Try 75.2.172.223 :)

[Ben Englert]

I could get there just fine, the code is a little unorganized, can you formatt it(it would help alot).

Elaborate... what do you mean by format it?

[Tom Bottiglieri]

Why did you use integer math in control_system.c and floating point in drive_control.c. All assignments are being dropped into int variables, so multiplying by 1.30 is just going to put extra strain on the processor to just get the same answer.

Try
targetticks = (percentage * 26) /20;

Yay for math by multiples of 2.

Another general question for all of the PIC gurus out there.. which would be more efficient, if there is any difference?
targetticks = (percentage * 26) /20;

or...

targetticks = (percentage * 13) /10;

[Matt Krass]

Try 75.2.172.223 :)

Still can't connect. Argh! Can somebody email me the file?

Email is my CD username, no spaces all lower case, and it's a gmail.com account.

[Ben Englert]

Why did you use integer math in control_system.c and floating point in drive_control.c. All assignments are being dropped into int variables, so multiplying by 1.30 is just going to put extra strain on the processor to just get the same answer.

Try
targetticks = (percentage * 26) /20;


That's something I've always wondered... people say that doing floating point math really does stretch the PIC to the edge of its capacity, but I haven't had any problems with it, even when it is handling a lot of interrupts from encoders etc. Is there an easy way to tell how much load it's under?

[TimCraig]

He's right in that in control_system.c your indenting is inconsistent. Quite a few more comments explaining why things are done and what functions actually do in plain English would be helpful.

I really don't like using multiple returns from a function. You're more likely to screw up your logic when you make changes with multiple returns and it's harder for outsiders to trace that logic. With the compiler warning level set high enough, and I really, really recommend you set it at the highest and fix your code so NO warnings are generated, functions like joystick_deadpan and percentage_limit will generate a warning because technically they are expected to return a value and when you just hit the closing curly brace you're returning void.

Some of these things may seem nit picking but they're what make the difference between good code and four letter word code. :yikes: And I've spent too much of my life trying to fix the latter kind.

[Ben Englert]

Okay, does anyone know which of the arguments for the Microchip compiler turn the warnings way way up?

I develop under Linux with a makefile so I can edit the arguments freely.

[TimCraig]

-w3 is the highest and gives "errors, warnings, and messages" -w1 is the lowest at errrors only. It looks like it defaults to -w2 because MPLAB doesn't show anything when I set that. That level is errors and warnings. I would use at least -w2 and then fix your code so there are no errors generated. If you don't, you and your successor will have to wade through a ton of warnings and may miss something important.

I'm not sure how stringent C18 is in generating warnings. If you get serious about it, you can run your code through a Lint program and generate a more comprehensive set of warnings. One place I used to work required we crank up the compiler and fix those warnings and then Lint it and fix those. Sometimes it was tough to make both happy but worth it.