Electronics > Microcontrollers
Could You Criticize my AVR C Code Please?
phil_jp1:
Hi everyone,
I wrote a firmware for watering system PLC-like controller and would be really glad if you could criticize my code.
My C programming skills is still at the level: "What soldering iron I should buy to program this microcontroller", and I need someone, more experienced embedded programmer to point out my major mistakes, so that I can avoid those mistakes in my future projects. Also in about a year, when my skills would be a bit better, I am going to start a major open-source software-hardware project, so this review will help not only me, but many other people.
I know that this is electronics forum, and many people here is on the same or even lower level, but there is also some people here who have a lot of experience in embedded systems and can really help in this case.
Anyways, here's my project:
https://github.com/jumperone/Watering-Sys-Ctrl
You can watch content of all the files online, or you can download it to your local machine. Well, it's git repository, nothing unusual.
p.s. The controller has 10 discrete optically-isolated 12V DC inputs, two optically-isolated triac drivers with zero-cross detection (MOC3062), one relay output and 5 non-isolated LED outputs.
Currently(prototype) it based around ATmega168 controller, but that might change. And this would be non-commercial one-off device. I also hope to write some tutorial on its hardware.
Thanks in advance,
Phil
Psi:
I had a quick browse and it looked pretty well written.
Better than some of my code :P
TerminalJack505:
I looks really good to me. I've seen worse. Even from so-called professional software developers.
You might want to document the MCU's pin assignment and fuse programming.
Here's an example from one of my projects:
--- Code: ---//
// This project uses an ATmega 324PV-10AU MCU.
//
// Pin assignments...
//
// Inputs:
//
// Front Panel Button Pins (Active low.)
// =====================================
//
// 20 BTN0 (PC1/PCINT17) Time Set. Internal pull-up resistor enabled.
// 19 BTN1 (PC0/PCINT16) Speed. " " " "
// 16 BTN2 (PD7/PCINT31) Intensity. " " " "
// 15 BTN3 (PD6/PCINT30) Down. " " " "
// 13 BTN4 (PD4/PCINT28) Up. " " " "
//
// Rear Panel DIP Switch Pins (Active low.) Internal pull-up enabled on each.
// ===========================================================================
//
// 21 CFG0 (PC2/PCINT18) 12 Hour Clock (on) / 24 Hour Clock (off)
// 22 CFG1 (PC3/PCINT19) Dual Intensity Enable (on) / Disable (off)
// 23 CFG2 (PC4/PCINT20) Hourly Chime Enable (on) / Disable (off)
// 24 CFG3 (PC5/PCINT21) Not used at this time.
//
// Note that when these pins are LOW the switch is in the ON position.
//
// Other Input Pins
// ================
//
// 30 AMB (PA7/ADC7) Ambient Light Detector.
// 12 LPM (PD3/INT1) Low Power Mode (High indicates battery operation.)
//
// Outputs:
//
// High-side Driver Control Pins (Active low.)
// ===========================================
//
// 42 HSC0 (PB2) High-side Control Channel 0.
// 41 HSC1 (PB1) High-side Control Channel 1.
// 36 HSC2 (PA1) High-side Control Channel 2.
// 44 HSC3 (PB4) High-side Control Channel 3.
// 43 HSC4 (PB3) High-side Control Channel 4.
// 37 HSC5 (PA0) High-side Control Channel 5.
//
// Low-side Enable Pins (Active low.)
// ==================================
//
// 35 LS0 (PA2) Low-Side Enable Channel 0.
// 34 LS1 (PA3) Low-Side Enable Channel 1.
// 33 LS2 (PA4) Low-Side Enable Channel 2.
// 9 LS3 (PD0) Low-Side Enable Channel 3.
// 10 LS4 (PD1) Low-Side Enable Channel 4.
// 11 LS5 (PD2) Low-Side Enable Channel 5.
//
// Other Outputs:
//
// 14 BUZZ (PD5/OC1A) Pulse Width Modulated Output.
//
// Other Pins:
//
// 4 Reset No programming required.
// 1 MOSI (PB5) Set as input w/ pull-up resistor enabled.
// 2 MISO (PB6) " " " " "
// 3 SCK (PB7) " " " " "
// 6 GND
// 18 GND
// 28 GND
// 39 GND
// 5 VCC
// 17 VCC
// 38 VCC
// 27 AVCC
// 25 TOSC1 Handled by the RTC.
// 26 TOSC2 Handled by the RTC.
// 7 XTAL1 Not connected.
// 8 XTAL2 " "
// 29 AREF " "
// 40 PB0 Set as input w/ pull-up resistor enabled. Not Connected.
// 32 PA5 " " " " "
// 31 PA6 " " " " "
//
// Note that the ISP pins are soley dedicated to ISP.
//
// Fuse programming:
//
// lfuse = 0xe2 (11100010)
//
// Divide clock by 8 bit unprogrammed.
// Clock output bit unprogrammed.
// Default startup time.
// Default clock source (internal RC oscillator.)
//
// hfuse = 0xd1 (11010001)
//
// On chip debugger is disabled. *
// JTAG is disabled.
// SPI programming is enabled.
// Watchdog timer alway on feature is off.
// EEPROM memory is preserved through a chip erase.
// Default boot size.
// Select Reset Vector bit (BOOTRST) is unprogrammed.
//
// * Disabling the on chip debugger is important! This feature, if
// enabled, will use a substantial amount of power even while in
// sleep mode.
//
// efuse = 0xff (11111111)
//
// Brown-out detector is disabled.
//
--- End code ---
It might be a good idea to stick the relevant component datasheets in a subdirectory of the project as well if they aren't already saved with the schematic/layout files.
vk6zgo:
On this forum,that's a bit like standing there with a "kick me" sign on your back! ;D
It looks like you got all the nice guys,though!
McMonster:
Just some strange code conventions. Like function and define naming and things in types.h. Is defining "u08" instead of just using "uint8_t" just for convenience?
Aside from that I haven't found any strange or ugly things in the code. But I'll look closer into this later, the PLC part seems interesting.
Navigation
[0] Message Index
[#] Next page
Go to full version