Hello,
I have this project : Wokwi - Online ESP32, STM32, Arduino Simulator
Is it possible to make this more maintainable to use multiple files
And if so, how can I look like ?
This is how you correctly post code on this forum:
// Alle pinnen van de leds
const uint8_t ledPins[] = { 9, 11, 10, 6, 3, 5 };
// de pin van de potmeter
byte potPin = A3;
byte switchPin = 12;
// de waarde die de potmeter aangeeft
int potValue;
int switchValue;
bool LED0 = 0;
// geeft de richting aan waar de led moeten draaien.
int richting;
// houdt de index bij van de huidige brandende led
int currentLed = 0;
// houdt de index bij van de leds van de staart
byte trails[5] = {0, 0, 0, 0, 0};
String action;
int oldSwitchValue;
unsigned long previousMillis = 0;
long interval;
void setup() {
// Zet de seriele monitor aan
Serial.begin(115200);
for (uint8_t cnt = 0; cnt < sizeof(ledPins); cnt++) {
// Zet alle leds uit
analogWrite(ledPins[cnt], 0);
// Zet de pin mode op output
pinMode(ledPins[cnt], OUTPUT);
}
pinMode(switchPin, INPUT_PULLUP);
}
void debug(String text, int variable) {
Serial.print("Mode :");
Serial.print(action);
Serial.print('\t');
Serial.print('\t');
Serial.print("Led :");
Serial.print(currentLed);
Serial.print('\t');
Serial.print('\t');
Serial.print(text);
Serial.print(" : ");
Serial.print(variable);
Serial.print('\t');
Serial.print("Richting");
Serial.print(" : ");
Serial.println(richting);
}
void Collect() {
/**
Reads the value of the pot meter
@param values none
@return void because only a local variable is being changed
*/
potValue = analogRead(potPin);
switchValue = digitalRead(switchPin);
}
void reset() {
/**
reset all values to the starting values
*/
if (switchValue == 0) {
currentLed = -1;
} else {
currentLed = -2;
}
for (int i = 0; i < 5; i++) {
trails[i] = 0;
}
for (uint8_t cnt = 0; cnt < sizeof(ledPins); cnt++) {
// Zet alle leds uit
analogWrite(ledPins[cnt], 0);
}
}
void Process() {
/**
Change value of currentled
Changes the value of some local variables.
@param values none
@return void because only a local variable is being changed
*/
if (switchValue != oldSwitchValue) {
reset();
oldSwitchValue = switchValue;
if (switchValue == 0) {
action = "staart";
currentLed += richting;
for (int i = 4; i > 0; i--) {
trails[i] = trails[i - 1];
}
trails[0] = currentLed;
}
if (switchValue == 1) {
action = "twee_keer";
currentLed = 0;
LED0 = !LED0;
}
}
// Set to predefined values
richting = 0;
interval = 5000;
// 512 - 30 (middle value - offset)
if (potValue < 482)
{
// richting wordt linksom
richting = -1;
// berekenen van de nieuwe interval voor linksom
interval = map(potValue, 482, 0, 500, 75);
}
// 512 + 30 (middle value + offset)
if (potValue > 542) {
richting = 1;
// berekenen van de nieuwe interval voor rechtsom
interval = map(potValue, 1023, 542, 75, 500);
}
debug("switchValue", switchValue);
}
void display_leidend() {
digitalWrite(ledPins[currentLed], LED0);
}
void display_volgend() {
if (action == "staart") {
analogWrite(ledPins[trails[4]], 0);
analogWrite(ledPins[trails[3]], 25);
analogWrite(ledPins[trails[2]], 54);
analogWrite(ledPins[trails[1]], 117);
}
if (action == "twee_keer") {
digitalWrite(ledPins[1], LED0);
digitalWrite(ledPins[2], !LED0);
digitalWrite(ledPins[3], !LED0);
digitalWrite(ledPins[4], LED0);
digitalWrite(ledPins[5], LED0);
}
}
void Display() {
/**
takes care of de display of leds
@param values none
@return void because nothing changes
*/
display_volgend();
display_leidend();
}
void preventOverflow() {
/**
take care that there is no overflow
@param values none
@return void because only a local variable is being changed
*/
if (richting == 1) {
if (action == "staart") {
if (currentLed >= sizeof(ledPins) - 1) {
currentLed = -1;
}
}
}
if (richting == -1) {
if (action == "staart") {
if (currentLed <= 0) {
currentLed = sizeof(ledPins);
}
}
}
}
bool timer() {
unsigned long currentMillis = millis();
if (currentMillis - previousMillis >= interval) {
previousMillis = currentMillis;
return true;
}
return false;
}
void loop() {
if (timer()) {
Collect();
Process();
Display();
preventOverflow();
}
}
It's only just over 200 lines. I don't think splitting it into multiple files would make it more maintainable, maybe less.
I would recommend learning about enum types to replace your inefficient use of String.
oke
I have learned a little about enums
I thought about maybe using both
I ask because I do not like the many if-then's on some functions
Yes.
Do this to each function:
- Select each function,
- cut the function,
- open a new tab in the IDE (or wokwi)
- name the new tab "myNewfunction.h"
- paste the function into its tab.
- add a
#include "myNewfunction.h"line to your main sketch
All those functions you moved need to have access to the global variables during compiling... so all those new #include lines must follow the global variables. Looks like this (in wokwi)
Could use a couple of enums (these are anonymous -- saves you from inventing a type name, if you don't need it)
enum {left = -1, stop, right} richting;
enum {neither, staart, twee_keer} action;
As global variables, they default to zero; the same as the value for the first option of each enum. So only one has to be assigned explicitly.
Use else if to avoid checking related conditions that cannot possibly be true
if (action == staart) {
analogWrite(ledPins[trails[4]], 0);
analogWrite(ledPins[trails[3]], 25);
analogWrite(ledPins[trails[2]], 54);
analogWrite(ledPins[trails[1]], 117);
} else if (action == twee_keer) { // not evaluated if `staart`
digitalWrite(ledPins[1], LED0);
digitalWrite(ledPins[2], !LED0);
digitalWrite(ledPins[3], !LED0);
digitalWrite(ledPins[4], LED0);
digitalWrite(ledPins[5], LED0);
}
Some of the checks can be reorganized to be shorter, but more importantly they have make sense when anyone -- including you -- reads this code in six months.
if (action == staart) {
if (richting == right && currentLed >= sizeof(ledPins) - 1) {
currentLed = -1;
} else if (richting == left && currentLed <= 0) {
currentLed = sizeof(ledPins);
}
}
(Not optimal -- if that matters -- but maybe it reads better. Interesting that the assigned "wrap around" values are "outside the edges".)
One minor problem with enums is that if you just print them -- as you do in the debug function -- you get an integer, not the "name", which has been compiled away. There are workarounds if that bothers you.
Thanks all,
I was thinking more about giving each effect there own class so something like this :
#include "led_effect.h"
#include "effect_staart.h"
#include "effect_twee_keer.h"
const uint8_t ledPins[] = { 9, 11, 10, 6, 3, 5 };
const byte potPin = A3;
const byte switchPin = 12;
LEDEffect* effect = nullptr;
StaartEffect staart;
TweeKeerEffect tweeKeer;
int potValue;
int switchValue;
int oldSwitchValue;
int direction = 1;
unsigned long previousMillis = 0;
long interval = 250;
void setup() {
Serial.begin(115200);
for (uint8_t i = 0; i < sizeof(ledPins); i++) {
pinMode(ledPins[i], OUTPUT);
}
pinMode(switchPin, INPUT_PULLUP);
effect = &staart; // Start met staart
}
void loop() {
unsigned long now = millis();
if (now - previousMillis >= interval) {
previousMillis = now;
potValue = analogRead(potPin);
switchValue = digitalRead(switchPin);
if (switchValue != oldSwitchValue) {
oldSwitchValue = switchValue;
if (switchValue == 0) {
effect = &staart;
} else {
effect = &tweeKeer;
}
effect->reset();
}
direction = (potValue < 482) ? -1 : (potValue > 542) ? 1 : 0;
effect->update(potValue, direction);
effect->display(ledPins);
effect->tick();
}
}
#ifndef LED_EFFECT_H
#define LED_EFFECT_H
#include <Arduino.h>
class LEDEffect {
public:
virtual void reset() = 0;
virtual void update(int potValue, int direction) = 0;
virtual void display(const uint8_t ledPins[]) = 0;
virtual void tick() = 0;
virtual ~LEDEffect() {}
};
#endif
#ifndef EFFECT_STAART_H
#define EFFECT_STAART_H
#include "led_effect.h"
class StaartEffect : public LEDEffect {
private:
int currentLed;
uint8_t trail[5];
public:
StaartEffect();
void reset() override;
void update(int potValue, int direction) override;
void display(const uint8_t ledPins[]) override;
void tick() override;
};
#endif
#include "effect_staart.h"
StaartEffect::StaartEffect() {
reset();
}
void StaartEffect::reset() {
currentLed = -1;
for (uint8_t i = 0; i < 5; i++) trail[i] = 0;
}
void StaartEffect::update(int potValue, int direction) {
currentLed += direction;
for (int i = 4; i > 0; i--) {
trail[i] = trail[i - 1];
}
trail[0] = currentLed;
}
void StaartEffect::display(const uint8_t ledPins[]) {
const uint8_t brightness[] = {117, 54, 25, 0};
for (uint8_t i = 1; i < 5; i++) {
analogWrite(ledPins[trail[i]], brightness[i - 1]);
}
digitalWrite(ledPins[currentLed], HIGH);
}
void StaartEffect::tick() {
// Eventuele animatie of timing hier (nog leeg)
}
With some help of AI to make the virtual class
What do you experts think of this idea ?
I think you will not learn without creating this yourself.
How is this different from tabs/helper files? (post 5)
There every function has his own file so with adding new effects I will take some more files and on the per effect as class I have to add one file.
With your idea is it easy to re-use some functions and with my idea the files are on all classes so no re-use of functions if needed.
And on your idea the .info files still have a lot of code where I will try to have only the setup and loop into the .info file.