Rangeerpuzzel (Inglenook) met Arduino besturing

[Timo]

Hoi Cees,

Ziet er goed uit!  Documentatie is altijd fijn Zou ik je toch een paar tips mogen geven? Zie het vooral dus niet als negatief iets maar puur als hulp! 

Code: [Selecteer]

if (snelheid < -1) snelheid = -1; else snelheid = speed_byte;
Van die regel gaan mijn rugharen een beetje overeind staan  Voor de leesbaarheid, regels eindigen na de ;. Omdat het een simpel statement is zou je er wel een one line'er van kunnen maken maar dan als:

Code: [Selecteer]

(snelheid < -1 ? snelheid = -1: snelheid = speed_byte);
Maar meestal gaat mijn voorkeur toch gewoon uit naar voluit omdat het vaak duidelijker is en makkelijker uit te breiden.

Overigens, wil je daar niet van richting veranderen als je op de andere richting drukt? Dus:

Code: [Selecteer]

if (snelheid < -1)
  snelheid = 1;
else
  snelheid = speed_byte;
Dus als hij achteruit rijdt en je druk op vooruit wel de richting omdraaien maar niet rijden.

Dan een blokje variabele namen (klinkt heftig, bedoel het goed!)
Als eerste je naamgeving. Goed, het staat je vrij om een bepaalde schrijfstijl te gebruiken, dat is verder geen punt. (Standaard in C is camelCase, en dat gebruik je soms opeens ook. Probeer voor één stijl te gaan.) Maar niet alle variabele namen zijn altijd even duidelijk. Nu weet jij dat DK voor drukknop staat maar dat hoeft niet gelijk duidelijk te zijn. Of, in mijn ogen, de ergste, lok. Het verwijst naar een array met lok adressen, iets wat niet duidelijk is aan de naam. Beter een lange duidelijke naam dan een korte onduidelijke. Dus locAdressen (loc_adressen) is dan wel wat langer maar geeft gelijk weg wat het doet. En een veel gebruikte verduidelijking is het in meervoud schrijven als het een array betreft (adressen in dit geval). En speed_byte snap ik de toevoeging byte niet. Het is gewoon de snelheid in 8-bit, niet eens als byte want het is signed. snelheid of locSnelheid zou hier genoeg zijn (want waarom hiervoor opeens Engels?).

Code: [Selecteer]

const byte lok1 = 99;
const byte lok2 = 3;
const byte lok3 = 3;
const byte lok4 = 25;
/* Einde lokadressen  invoeren */

byte lok[] = {lok4, lok3, lok2, lok1};
Waarom niet gelijk de array vullen? Dan heb je geen overhead aan variabele en bespaar je jezelf typewerk. Nu ben je gewoon werk dubbel aan het doen, altijd zonde 

En locIndex heeft geen reden om global te zijn. Dus gewoon:

Code: [Selecteer]

     pinMode(locAdresPinnen[0], INPUT_PULLUP);
     pinMode(locAdresPinnen[1], INPUT_PULLUP);
     byte lokIndex = digitalRead(locAdresPinnen[0]);  // index wordt 0 of 1
     lokIndex += 2 * digitalRead(locAdresPinnen[1]); // index krijgt er 0 of 2 bij
     lokAdres = lok[lokIndex];
Gelijk ze een naam gegeven, naamloos is nooit echt duidelijk  Of meer schaalbaar (mocht je uit willen breiden):

Code: [Selecteer]

byte locIndex = 0;
for(byte i = 0; i < sizeof(locAdresPinnen); i++){
  locIndex <<= 1;
  pinMode(locAdresPinnen[i], INPUT_PULLUP);
  locIndex |= !!digitalRead(locAdresPinnen[i]);
}
Edit Ik zie dat je de index gebruikt voor eeprom. Ik zou dan alleen de index opslaan en adres weg doen en dan adres altijd ophalen uit de lijst. Zo heb je niet twee variabele die eigenlijk beide naar het adres moeten wijzen maar per ongeluk toch verschillend zouden kunnen zijn.

Code: [Selecteer]

char speed_byte, old_speed = 0, snelheid = 1;
Zoals je zelf ook gemerkt hebt is char niet echt het handigste type om hier te gebruiken. Dit omdat char door veel functies afgehandeld wordt als een ASCII karakter.  Ik zou het vervangen door int8_t. Is ook gewoon signed 8-bit maar wordt niet gelijk gezien als karakter.

Code: [Selecteer]

void stel_snelheid_in()
{
Paar opmerkingen over die functie.

Waarom iedere keer de led high maken? Deze kan buiten de while.

Wat is het hele idee van de variabele old_speed? Is dat alleen maar om te zorgen dat je niet op 0 kunt instellen? Als dat zo is, aangezien je toch niet als snelheid 0 zou willen hebben als ingestelde snelheid, kan je het dan niet veel makkelijker gewoon

Code: [Selecteer]

if(speed_byte == 0){
  speed_byte = 1; //richting maakt niet uit, 0 wil je toch niet en nu heb je één extra stapje voor "0 vooruit"
}
Ook heeft old_speed geen enkel recht om global te zijn 

Code: [Selecteer]

speed_byte = (analoge_waarde >> 2)-127 ;
Besef dat je dit ook ongewenst -127 als snelheid kan geven.

Code: [Selecteer]

  if (speed_byte < 0) speed_byte = -speed_byte;
  EEPROM.write(lokIndex, speed_byte);
  delay(500);                             // wacht tot zeker is dat EEPROM beschreven werd
Nergens voor nodig  Komt gewoon in EEPROM. Je had ook abs() kunnen gebruiken.

Je zou ook tijdens het instellen van de snelheid de optrek vertraging uit kunnen zetten. Kan je het allemaal net wat sneller zien of het idd de juiste snelheid is.



Goed, weer erg lange post. Absoluut niet negatief bedoelt, vind het een leuk project!  Maar dit was wat me op viel toen ik het las.

Timo

[Cees Baarda]

@Timo
fijn dat je weer meegelezen hebt . Ik heb nog wat erfenissen van de voorbeeldsketch laten staan. Het aardige is dat ondanks een paar missers, zoals het direct omkeren van de rijrichting als je de andere richtingknop indrukt e.e.a. al een aantal dagen probleemloos blijft werken. Je hebt me aan het werk gezet met stijl en leesbaarheid aanpassen en een aantal zaken consequenter maken. Ga ik zo maar eens aan beginnen.
Wat betreft de "overkill" aan benamingen:
- Als ik er een handleiding bij maak, wil ik graag dat degene die deze sketch gebruikt duidelijke plekken heeft om bijv. de lokadressen in te vullen. Het is veel lastiger op papier uit te leggen, dat iemand de 4 adressen in omgekeerde volgorde in een arraydefinitie moet stoppen.
- Verder wil ik verderop in de file liever geen niet benoemde zaken zetten. Dus A0, A1 en A2 krijgen wat mij betreft ook nog namen, die iets zeggen over de functie.
E.e.a. levert wel meer tikwerk op, maar maakt het hopelijk ook veel duidelijker voor iemand, die veel minder programmeert dan wij.

groet Cees

[Timo]

Hoi Cees,

Hehe, ja, als je code van andere gebruikt ben je altijd even bezig de boel wat leesbaarder te maken ja. Tenminste, als je zelf duidelijke en mooie code wilt overhouden. Goed bezig dus!

Wat betreft duidelijke plekken, de extra namen zou ik nu juist als verwarrend zien. En waarom zou dat in omgekeerde volgorde moeten? Wat is er mis met iets als

Code: [Selecteer]

//Hier de locadressen invullen
byte locAdressen[] = {60,  //Adres 1
                      45,  //Adres 2
                      67,  //Adres 3
                      32}; //Adres 4

En wat betreft niet benoemde zaken, totaal mee eens. Door het bovenaan te noemen kan je het snel terug vinden en eventueel wijzigen zonder alle code door te spitten. Ook is bij het gebruik dan sneller duidelijk wat je precies doet.

E.e.a. levert wel meer tikwerk op, maar maakt het hopelijk ook veel duidelijker voor iemand, die veel minder programmeert dan wij.

Niet alleen voor iemand die minder programmeert. Maar zeker ook voor future you. Als je een project een jaar in de kast heb laten liggen (af of niet  ) is het altijd een stuk makkelijker wat aanpassen als de code duidelijk, gestructureerd en becommentarieerd is.


Timo

[Cees Baarda]

Dag Timo (en anderen)

Wat betreft duidelijke plekken, de extra namen zou ik nu juist als verwarrend zien. En waarom zou dat in omgekeerde volgorde moeten? Wat is er mis met iets als

Code: [Selecteer]

//Hier de locadressen invullen
byte locAdressen[] = {60,  //Adres 1
                      45,  //Adres 2
                      67,  //Adres 3
                      32}; //Adres 4


Waarom had ik gekozen voor losse declaraties:
1 - Eigenlijk om het echte programmagedeelte helemaal gescheiden te houden van de invulactie.
2 - Als ik in een handleiding het ga hebben over lok 0 t/m lok 3, zou ik niet willen uitleggen waarom men achterste voren moet tellen. Immers door mijn hardware-oplossing {als er geen schakelaar aan de lokKeuzePinnen zit} zijn de pinnen hoog voor de situatie waar de BCD-schakelaar 0 aangeeft.
3 - Door mijn opzet is het minder waarschijnlijk dat iemand, die gaat invullen het array aanvult met bijv. een 5e lok zonder verderop het inlezen van BCD-pins te veranderen. En daarna zich gaat afvragen waarom die 5e lok niet gekozen kan worden. Wellicht begrijp je nu waarom ik voor de langere weg heb gekozen.

Jouw oplossing met de andere opzet van het array is schitterend . Als dan ook bij het inlezen van de 2 pinnen de telling omgekeerd wordt (zie de code) dan zou het allemaal ook gaan kloppen.

Code: [Selecteer]

    huidigeLok = digitalRead(lokKeuzePin1);                // index wordt 0 of 1
     huidigeLok = huidigeLok + 2 * digitalRead(lokKeuzePin2); // index krijgt er 0 of 2 bij
     lokAdres = 3 - lokAdressen[huidigeLok];   // keer de telling om
Maar door de keuze van aparte constanten voor de lokAdressen kon ik dat al oplossen in de array declaratie.

Ik heb inmiddels de hele file aangepast aan een stijl met o.a. "kamelenKaas" en geen Engelse namen (eigenwijze keus en beter leesbaar voor al die figuren, die zelfs geen Engels beheersen). Ik post die file maar niet meer hier. Zodra de uitbreiding van mijn site een feit is, zal die daar komen te staan.

groet Cees

[Karst Drenth]

Code: [Selecteer]

if (snelheid < -1) snelheid = -1; else snelheid = speed_byte;
Van die regel gaan mijn rugharen een beetje overeind staan Voor de leesbaarheid, regels eindigen na de ;. Omdat het een simpel statement is zou je er wel een one line'er van kunnen maken maar dan als:

Code: [Selecteer]

(snelheid < -1 ? snelheid = -1: snelheid = speed_byte);
Maar meestal gaat mijn voorkeur toch gewoon uit naar voluit omdat het vaak duidelijker is en makkelijker uit te breiden.

Overigens, wil je daar niet van richting veranderen als je op de andere richting drukt? Dus:

Code: [Selecteer]

if (snelheid < -1)
  snelheid = 1;
else
  snelheid = speed_byte;


Van deze:

Code: [Selecteer]

(snelheid < -1 ? snelheid = -1: snelheid = speed_byte);

...gaan mij de haren overeind. Als je dan toch les wilt geven in on-liners, dan zo:

Code: [Selecteer]

snelheid = (snelheid < -1) ? -1 : speed_byte;

oftwel een conditional assignment statement Deze vorm van 'if-then-else' heeft als groot voordeel, dat je de 'else' niet kunt vergeten. Het is overduidelijk, dat 'snelheid' altijd een waarde moet krijgen. De geeigende manier is dan het conditional assignment statement


Grtzz,

Karst

P.S.

Zou ik je toch een paar tips mogen geven? Zie het vooral dus niet als negatief iets maar puur als hulp! 

P.S.2. Trouwens leuk project Cees

[Timo]

@Karst, je hebt gelijk! Dat is netter. Ik moet toegeven dat ik zelf bijna nooit one liners gebruik (maar hier is hij best mooi zoals jij hem neer zet). Meestal schrijf ik ze uit omdat je altijd zult zien dat je precies daar later toch wat wil toevoegen. Daarom laat ik ook nooit de blockhaken weg bij een statement om niet te fout te maken iets toe te voegen en te denken dat hij in de statement zit. 

@Cees, Ik snap je redenering wel maar door de opzet is de schaalbaarheid (en ik vind ook de duidelijkheid) lager. Even punt voor punt 
1) Tja, je bent toch al in code bezig. De hele array declaratie kan toch ook al in het invuldeel staan?
2) Simpel op te lossen, lees gewoon de switch inverse En klaar is Kees Cees
3) Dat valt tegen denk ik  Mensen zonder programmeerkennis zullen net zo makkelijk gewoon een variabele lok5 toevoegen en verwachten dat het werkt. 

En de hele BCD loc selectie zou je natuurlijk al uit kunnen breiden als je de pinnen hebt. Uitgaande van een BCD schakelaar is dan 4 pinnen. (en zou je zelfs tot 16 locs kunnen selecteren als je jumpers doet.) Als je altijd gewoon 4 pinnen leest ben je altijd veilig als je meer adressen toevoegt.

Code: [Selecteer]

//Hier de locadressen invullen
byte locAdressen[] = {60,  //Adres 1
                      45,  //Adres 2
                      67,  //Adres 3
                      32}; //Adres 4

const byte LokKeuzePinnen[] = {A1, A2, A3, A4};

byte locIndex;


void kiesLoc(){
  locIndex = 0;
 
  for(byte 1 = 0; i < sizeof(LokKeuzePinnen); i++){
    pinMode(LokKeuzePinnen[i], INPUT_PULLUP);
   
    //schuif eens op
    locIndex <<= 1;
   
    //Inverse lezen om het omdraaien van de loc adressen ongedaan te maken.
    locIndex |= !digitalRead(LokKeuzePinnen[i]);
  }
}

inline byte locAdres(){
  //Als er een te hoge geselecteerd wordt geef gewoon de 1e.
  if(locIndex >= sizeof(locAdressen)){
    return locAdressen[0];
  }
  return locAdressen[locIndex];
}
Let wel op dat hierdoor locKeuzePinnen[0] MSB is van de index. Wil je dat niet dan

Code: [Selecteer]

    locIndex |= !digitalRead(LokKeuzePinnen[sizeof(LokKeuzePinnen) -1 - i]);
Om de array andersom te gebruiken.


Timo

[Cees Baarda]

@Timo
Met alle respect voor je opmerkingen: het wordt nu te veel een discussie over schaalbaarheid. Dat is niet mijn bedoeling in dit project. Dan had ik wel aangekondigd e.e.a. in een library te gaan frotten.
Het programmaatje zoals ik het nu heb, laat m.i. wel zien hoe je zo'n simpele rijregelaar zou kunnen opbouwen. De andere stukjes, die je voorstelt schieten voor mij het doel een beetje voorbij. Ik laat het graag aan een ander over om het op zijn manier te doen. Onder mijn invulgedeelte zal ik het volgende commentaar gaan zetten.

Code: [Selecteer]

/* * * * * * O n d e r s t a a n d e   c o d e   a l l e e n  * * * *
 * * * * * * a a n p a s s e n  a l s  j e  v e r t r o u w d * * * *
 * * * * * * b e n t   m e t   C-p r o g r a m m e r e n   e n  * * * 
 * * * * * * A R D U I N O                                    * * * *
 */
En ergens bovenaan komt ook nog iets te staan dat iedereen vrij is te doen met dit stukje code wat men wil.

@iedereen
Ik heb van deze discussie wel weer iets geleerd:
- ik was nog niet consequent genoeg met de gebruikte (programmeer-) stijl.
- oneliners zijn voor mij achteraf gezien ook ondingen, vooral omdat ze minder goed leesbaar zijn.
- schaalbaarheid is een groot ding, maar moet niet het doel van het programmeren zijn.
- efficiente code is soms niet noodzakelijk.
- soms leiden er meer wegen naar Rome, waarbij de kortste of beste niet altijd te prefereren valt.
- alvast vooruit denken naar een handleiding levert niet altijd de mooiste code op.

Ik ga vrolijk verder met dit project. Komende week verwacht ik mijn volgende printjes. Dan ga ik een echte rijregelaar opbouwen. En dan beginnen met de printjes, die ik al heb om servo's aan te sturen met schakelaars. Tussendoor baantje nog aanvullen met een uithaalspoor.
De posts worden misschien even wat minder omdat ik een workshop over iets heel anders bij HCC!m ga voorbereiden.

groet Cees

[Timo]

Hoi Cees,

Allereerst probeer ik geen discussie aan te gaan! En is en blijft jou project, probeerde alleen wat tips te geven. En ik weet dat het af en toe wat "dringend" over kan komen op een forum vandaar dat ik het niet vaak genoeg aan kan halen en niet genoeg smileys in mijn berichten kan verwerken. 

Mijn opmerking was ook niet zo zeer voor schaalbaarheid of code efficiëntie bedoelt maar om gebruikers fouten op te vangen. Wat ik zei, iemand zonder kennis voegt net zo makkelijk een loc5 toe en verwacht dat het werkt  Maar ook om je te laten zien dat die "omgekeerde volgorde" geen dingetje is. Ik weet dat je af en toe met code je zo vast kunt kijken op iets (mm, ik moet de array omgekeerd maken) terwijl iemand anders met een verse blik denkt van, "mm, je zou de locIndex ook gewoon kunnen omdraaien." Net als Karst met een veel betere one-liner kwam enz. En ja, die "iemand anders met een verse blik" is meer dan eens je toekomstige zelf 

Dus nogmaals, niets negatiefs bedoelt! Het waren maar "my two cents" over de punten die je noemde.  Je noemt namelijk zelf al een punt wat ik ook niet vaak genoeg aan kan halen op het forum:

- somser leiden meer wegen naar Rome, waarbij de kortste of beste niet altijd te prefereren valt.

Maar ik vind het dan wel altijd fijn om een paar wegen te kennen zodat je de voor en nadelen van een route vooraf kunt vaststellen 

Veel succes weer verder met het project! Blijf het natuurlijk volgen en hoop dat je mijn opmerkingen luchtig blijft lezen 

Timo

PS. Misschien had ik het al gevraagd maar ben ik het vergeten , maar wilde je alles openbaar maken (GitHub bijvoorbeeld)?

[Cees Baarda]

In post #55 had ik een foto gezet van mijn proefopstelling waar nog een breadboard gebruikt werd om de APM te huisvesten. Inmiddels zijn mijn "universele" printjes binnengekomen met een grappige en een slechte ontwerpfout. Ik heb er toch 1 in elkaar gezet om van dat braadbord af  te komen. De opstelling is nu zoals op de foto hieronder. Links de voedingenprint (de laptopvoeding heb ik weggelaten). Dan de "universele print" met een minimum bezetting. Daarboven een BNLS/RCU boosterprint. Rechts nog een noodprintje met drukknoppen. Naast de APM zie nog een duimwielschakelaartje om de keuze 0 t/m 4 van loc's te kunnen maken. Met dit alles valt er inmiddels te rijden op het baantje. Nu maar eens aan de servo-besturing beginnen.



edit: laatopvoeding moest zijn laptopvoeding (correctie van mijn MAC over het hoofd gezien)
Grappige fout was dat de RJ12 connectoren naar de onderkant moesten. Achteraf misschien niet eens zo erg.
Slechte fout: de afbeelding van de 1N4148 diodes staat fout op de print.



groet Cees

[Raages]

Je bent weer een flinke stap verder en ach, een klein foutje mag. Toch? 

[Timo]

Ziet er weer goed uit Cees!

Maar hoe is dat met de diode gebeurd? Net even gekeken en bij mij hebben zowel alle components (van de standaard device library) als alle footprints (vand e standaard throughhole library) de kathode op pin 1 zitten. Welk component en welke footprint heb jij gebruikt?

En de RJ12, was dat echt foutje doordat je hem aan de verkeerde kant van de print geplaatst hebt of klopte die footprint niet?


Timo

[gvandersel]

IK ben ook benieuwd. Bij mij zitten de tekeningen op de print van de 1N4148 ook verkeerd om. Gebruikte combinatie:
- standaard diode
- Walter footprint en 3D weergave (DO35).

Ben al enige tijd aan het zoeken.

Groet,

Gerard van der Sel.

[Cees Baarda]

@Timo,
de footprint van de RJ12 had ik gekopieerd uit w_con_misc van deze website. Daarna alleen de outline van de component wat aangepast. In het origineel zitten de 6 eilandjes al gespiegeld. Ga ik later nog wel aanpassen, door een onder en boven de print versie te maken. En nog even goed kijken of de padnummers blijven kloppen met het schema-symbool.

@Gerard
de pinnummering van de footprint moet overeenkomen met die van het schema-symbool. Ik heb het inmiddels in mijn eigen footprint aangepast. Kwestie van boven het padje gaan staan in de footprint-editor met je muis E intoetsen en nummer veranderen. En bewaren. Dan nog in je printlayout bij de betreffende footprint Change footprint kiezen, niets veranderen en OK klikken. Daarna mag je opnieuw bedraden of positioneren.

groet Cees.

[Timo]

@gvandersel, Ahh, kijk, de Walter versie heeft gek genoeg pin 1 als Anode ipv, wat in de rest gebruikelijk is, kathode... Toen even een oude component van mij geopend en ja hoor, omgedraaid. Zowel de diode als de led zijn omgedraaid. Dit is dus een van de wijzigingen geweest in KiCad 4. Veel verbeteringen in 4 maar de overgang naar 4 is niet altijd even soepel geweest... Als ik nu ook probeer de adServo te openen gaat er het een en ander mis 

De diode moet ik dan ook weer even aan gaan passen. Ik had ook zelf een footprint gemaakt omdat ik 200mil tussen de pinnen van een 1N4148 genoeg ruimte vind.

@Cees, gek dat de RJ12 van Walter gespiegeld is, zie het nu ook ja. Nummers zouden moet blijven kloppen aangezien de connector nu op zijn kop werkt. Er is overigens geen enkele reden een boven en onder de print versie te maken. Gewoon een versie (voor boven de print). Wil je hem aan de andere kant van plaats je hem gewoon op de achterkant van de PCB en klaar.


Timo

[Cees Baarda]

@Timo

@Cees, gek dat de RJ12 van Walter gespiegeld is, zie het nu ook ja. Nummers zouden moet blijven kloppen aangezien de connector nu op zijn kop werkt. Er is overigens geen enkele reden een boven en onder de print versie te maken. Gewoon een versie (voor boven de print). Wil je hem aan de andere kant van plaats je hem gewoon op de achterkant van de PCB en klaar.

Zo heb ik dat nu gedaan en dat werkt voor LocoNet. Als je echter de connector ook anders wilt gebruiken, waarbij de 6 aders verschillend zijn, dan heb je wel 2 versies nodig.

groet Cees