aikopras / AP_DCC_Decoder_Core

Core library for DCC accessory decoders that use the RS-Bus for feedback. PoM feedback messages use (instead of RailCom) the RS-Bus with address 128.
GNU General Public License v3.0
0 stars 0 forks source link

code improvment suggestions(not issue) #2

Open LaurentR59 opened 1 year ago

LaurentR59 commented 1 year ago

Hi

I would suggest some improvments for the lib ( I did not find a way to request by another way sorry for that!)

A/ Ability to support MAGTINYCORE "officialy" with CPU like ATTINY3217 3216 3227... as economic basic (and available) CPU challengers. TINY are close similar as MEGACOREX or DXCORE. ( TIMER, ...)

B/ led class should have for "blink" an option to finish at HIGH or LOW state. Like this a NEON TUBE START can be easily emulate with end state LIGHT ON of OFF ( for defect run) or many other uses for non stop run or determined ones.

C/ ability to manage more than 1 adress as principal adress.: let me explain the target object: imagine a loco decoder but not for function motor engine but for an on board unmotorised function vehicule managing ligh led only. (no motor driving needs) If each object use a different "main" adress reference you will have many messages on DCC bus for the whole train. If you select the same adress for each vheicule, making a sort of "train adress", you could reduce DCC trafic messages only on 1 adress considered as "main" but no possibility to manage individual item ( all listen the same adress) can be call by a different adress as "primary or pricipal. On the "second main " adrress it should be possible to call light effects animations and managing pre set scenario. (ON/OFF animatic sequence) So my first think was to flag with one active function on one dedicated adress the adress used and DCC messages decoded. (for the rest and maanging priority of DCC message routing according this setting) It means It could request a "setMyAddress" in loop to swap on and process at next loop message on this "new second main adress". Needs shoule be like this form: (main)PRIMARY, CONSIST, TRAIN_GROUP1, TYRAIN_GROUP2, TRAIN GROUP3,... All this adresses are managed like CV17/18 ( except CONSIST on CV19) and can be load but for next step... it s clealy undefined! May it could be with CONSIST support next improvment for the LIB and easiest way to manage function decoder.

Hope you could consider thesse requirement for futur releases

Many thanks by advance Laurent.

LaurentR59 commented 1 year ago

Hi Aiko

Regarding the SUP_LOCO element may a way should be considering some minor changes like IsMyadress according an || with items like these:

myLocoAddressFirst; myLocoAdressSecond, myLocoAdressThird, ...

so it would become

return ((locoCmd.address >= myLocoAddressFirst) ||( locoCmd.address >=myLocoAdressSecond) || (locoCmd.address >= myLocoAdressThird)) && (locoCmd.address <= myLocoAddressLast));

So it shoud manage any message regarding each of these adresses.Right? (Priority management will be managed into the main programm according Adrress priority order selected)

Regards Laurent

aikopras commented 1 year ago

Dear Laurent

Thanks for your suggestions. Posting it here is fine, since that keeps all discussions connected with the code.

Support for the megaTinyCore processors might be easy, since they are indeed nearly identical to the DxCore. As first test it should be possible to just check if it compiles, after some additions are made to the files that define the processor type. Since I don't have any of these new AtTiny processors myself, I would not be able to check if it indeed runs as expected, however. Would you be able to perform such real-life testing? Some comments, however. On DxCore processors I use multiple timers, to improve performance and quality. The number of timers on the AtTiny might in some cases therefore become a problem.

The idea to add an option to finish at HIGH or LOW state is nice and should be easy to implement. I hadn't thought about that before, but it can be done.

Finally your suggestion to have multiple addresses for loco decoders. I do see the advantage you gave in your example. I thought (in the past) myself about possible light decoders in wagens, and having a single address for all wagens in a train would certainly be beneficial. At that time my idea was to include a Hall-sensor on each light decoder, and if activated by a magnet it would change its address. Your proposal to use consist addresses would certainly make the code more "standards compliant". I have to think a bit about this, and might implement it if there is interest in it and I have more time for implementation (in the next two months I'm travelling a lot and will have limited time, after that I would have time). Final note: such change would basically be an extension to the AP_DCC_LIB, and could probably be better discussed there. Bye, Aiko

aikopras commented 1 year ago

Hi Laurent

so it would become

return ((locoCmd.address >= myLocoAddressFirst) ||( locoCmd.address >=myLocoAdressSecond) || (locoCmd.address >= myLocoAdressThird)) && (locoCmd.address <= myLocoAddressLast));

Yes, that would certainly be an option. Implementing something like this might be trivial, but the downside is that such extended test should be made after the reception of each DCC message. Performance-wise that is likely OK, although I usually like to think about such things twice ;-)

Probably the more complex part is to handle the setting of the consist addresses, including CV values. Since my focus (until now) has been on stationary (accessory) decoders, I hadn't thought too much (yet) about loco decoders. Still I have somewhere plans to also built my own (light) function decoders. For them such extension would be quite useful.

Bye, Aiko

LaurentR59 commented 1 year ago

Dear Aiko

About MEAGTINYCORE I previsouly "forked" adding it on list of items from your lib. Compile without issue due to similar sctruture of DX CORE. Just timer are less than on AVR Dx or ATMEGA serie 0.

Probably the best way to test is to get the ATTINY3217 CURIOSITY NANO EVALUATION KIT

https://www.microchip.com/en-us/development-tool/EV50J96A

Same board exist with AVD Dx CPU so would be realy appreciate for breadbord test with hardware. Price is acceptable for this purpose but not really for PROD PCB layouts. Anyway CPU is a good alter part. Available in main provider like FARNELL, DIGIKEY, RS, MOUSER, ...

ATTINY 3217 ( from TINYAVR serie 1 and like serie 2 ) get 1 TCA and 2 TCB so would be just enough to main code. ( Seriie 0 get only 1 TCA and 1 TCB) ( ignored TCC or TCD here).

I forgot to add some minor changes on the same way on LOCO CLASS to set MyAdrress with additional elements

first is completed by second and third value ( additionnal adrress)

void Loco::setMyAddress(unsigned int first, unsigned int second, unsigned int third, unsigned int last) for exemple.

locoMessage.myLocoAddressFirst = first; locoMessage.myLocoAddressSecond = second; locoMessage.myLocoAddressthird = third;

( if second and third are not defined so it means they are = first) ...

That seems to be a way to test! ( Probably on late today)

You describe to do a circular refresh for each adress received somewhere but I could not see precisely where such element should be set/define.

My feeling is : per message transmited by a DCC controller with 1 adrress as TARGET we check adrress at receipt. If it matchs we process instuctions. We listen in that case for serevals destinations adrress Fisrt and Second, Third too as exemple. If DCC messages match with ones of them then we could process data message. ( Or may is too trivial? and not matching with internal treatments process? ( require some changes?))...

Probably the first step on DCC decoding process should determine the target adrress before any other step andd compare with ones yet set ( First, Second, Third...). So in main code we must perform some priorisation between instruction from the adress -: ex CONSIST if first priority else consider Third else consider Second and finaly First. For this case i don t consider yet CONSIST mode because that is a little bit different due to a DCC with specific sequence. Just focus on 3 adress. ( like first is one)

May i miss something on my trivial approach? :)

Regards Laurent

LaurentR59 commented 1 year ago

Hi all Tests done after some minor update to support serevals address on lib and get quite good succes on it! update was on following files: SUP_ LOCO. H SUP_LOCO.CPP AP_DCC_LIBRARY.H AP_DCC_LIBRARY.CPP

Why quite good result only?:

So now the decoder is able to listen (in my present test) to 3 different address (First Second and Third )(for exemple 3 4 and 6 as address values) Any change on one of them will be applied.

Anyway now it 's necessary to have some additional complementary items: Witch ones?

A/ when a change is applied we need to identify on witch address change happenned ==> will require to make a function to do it . With that dedicated variable item we can set in main programm mapping to a value for dedicated address usage or managing priority address usage.

B/ when a direction change is applied on one of the (3) decoder address it means a direction have been change for the whole lot of address, so need to have un update function for that whole lot too considering the 3 address are in fact only one.

That s my current result on that modifying step. Will post all details at end of update quite soon. (need to time to create such discribed functions/update)

Any advises are already welcome in!

Regards Laurent.

aikopras commented 1 year ago

Hi Laurent Although the changes you're making certainly make sense, I'm struggling with some more fundamental questions:

  1. Your proposal to add additional addresses is primarily intended for Multi-Function (Loco) decoders, or will they also be useful for accessory decoders? If the changes are only for Loco decoders, it should be added to the AP_DCC_LIB, but probably be "transparent" to the AP_DCC_Decoder_Core. The Core decoder library is primarily intended for accessory decoders with RS-Bus feedback. Such feedback is not needed / possible for Loco decoders.

  2. I'm not 100% sure what application you have in mind. I do understand your original example, where a train has several wagons and each wagen includes a decoder to switch lights. Is that still the idea? But do you want each wagen to be able to react differently to the "private address" (P) and the "shared" address (S)? In other words, should the user be able to "overrule" a F1 = ON to the shared address S by sending immediately after F1 = OFF to the private address P. I don't think that would be easy to implement, since light commands are repeated periodically and implementing the example above would require quite some administration. Still it seems from your posts that you want to implement that.

So let's first discuss what we want to achieve, and after that on how that should be implemented.

Bye, Aiko

LaurentR59 commented 1 year ago

Dear Aiko

Let me give more info on suggested changes. 1/ Target is for on board decoders (as "function" decoder for motorised or not engines) I did not see advantage for accessory at first glance but it should be considered for advanced accessory decoders that require more only than 1 DCC address ( I think directly on Light signal that can aggregate serevals DCC address for managing all case of display (around 30 for the most sofisticated SNCF signal as exemple) You are right the main evolution should be placed in the AP DCC LIB. (and be extended for use on all decoders as ACC or LOCO) I will make a link to this topic from the channel in AP DCC LIB.

2/ To describe more what is "expected" we should consider sereval address

The CONSIST ADDRESS case is a bit different address mode ( from the DCC Trame message the detection type is yet included) and should be the HIGHEST PRIORITY ADDRESS TYPE TO MANAGE. ( CV19 +CV20 and CV21 for managing function in consist)

Anyway for other cases:

this is the LOWEST PRIORITARY ADDRESS INSTRUCTION TO MANAGE.

All other address values are managed by the same model as CV17 + CV18 but obsiously with their own CV

-the "FOURTH" one is high priorrity (call it TOP PRIORITY)(more than previous and less than highest one ( CONSIST one)

In final approach we should decode address message for a decoder in this sequence if exist

HIGHEST(CONSIST ADDRESS) >TOP(FOURTH)>HIGH(THIRD)>MAIN(SECOND)>LOWEST(FIRST ADDRESS)

Ok so how to know we have to process one of them?

By selecting a "LOCK" if we encounter a specific condition. Witch one? In our case we should consider thant of F0 function is active on a "HIGHER" prioritary address. In that case then it s the auto selected adress to listen only by the decoder and process messages. Other messsages with less priorittary address are ignored. If no lock on HIGHER adress then process the next address prioritary till "FISRT".

To illustrate as demo of this behaviour consider address like this: FIRST is for individual managment ( not really usually use and disengaged by any higher address mode prioritry) SECOND is for the first part of a train that could be splited into 2 parts THIRD is for the second part of a train that could be splited into 2 parts FOURTH is the main train address that include all parts CONSIST is a way to link items (LOCO + COACH, LOCO +LOCO, LOCO+ TRAIN,...) and share function number states

I will let every one imagine whitch concepts in use should be possible playing this supported approach!

LaurentR59 commented 1 year ago

About my current tests: not really so good as expected. I have forked the APP to lest the decoeder listen to First Second and Third address. So any of them is interpreted by messages instruction!

I tried to make a lock filter with if(lococmd.address == First) or Second and third and check if F0 in locoCmd.F0F4 is in use or not.

That s not ok. And I missed my tests My analyse, at first glance, is that lock need to be at higher level at te decode process so from LIB directly...

May i m wrong but... not fixed for the moment.

Need to think mor on it to find a solution.

Regards Laurent

aikopras commented 1 year ago

Hi Laurent I'm not 100% sure what you are doing, but the check should (if my memory serves me well) be added to the AP_DCC_library in the file sup_loco.cpp. Look at LocoMessage::analyse(void). Note that several tests are done there, but the code should be readable enough to understand it.

Bye, Aiko