SF patch #3 opened 2015-06-16 moved to github: P300 setaddr patch

[hmueller01]

I fixed the P300 setaddr command, which send the data (SEND BYTES) after the checksum. Because framer_send() can be called several times before the message is complete, I moved sending the checksum to framer_receive() instead of framer_send() and using a local static variable to calculate the checksum step by step. Workes fine with my device ID="20CB" name="VScotHO1". Did not yet test the KW protocol again (hope my changes do not break that).

Also fixed that P300 code produced lots of logging output without debug (-g) set.

Please test the code and write results here. Hope that it finds it's way to the official trunk if everything works fine.

For more details and old feedback see also SF Patch #3

Unfortunately SF change r106 to r107 by fnobis in parser.c, execByteCode(), starting line 181 broke my setaddr patch again. For now I have undone this change. Need discussion with Frank, why he needed the change. But currently he is not active any more ...

Cheers, Holger

[l3u]

I think this should rather be merged with master, not with the cmake branch – I just moved files around there and changed the build system, but I changed no code …

I don't know (yet) what your patch does, but a nice thing would be if you separated changing code from formatting: if the patch that actually changes code is as minimal as possible, one can see what happens with a minimal effort. If changes like

-#define P300_REQUEST       0x00
+#define P300_REQUEST        0x00

are all over the place, it's a bit harder … no hard feelings! But I would do all the code formatting (what I proposed in https://github.com/openv/vcontrold/issues/1 ) in a separate commit!

[hmueller01]

Yes I did some kind of format code fixing. Some lines did have tabs others not, making things look ugly with my editor. I do not like this and I change it at once. But this VERY limited format change.

This code is very hard to read. Not because of the formatting. But of the way it is implemented. One gets used to all kind of spaces before or after a bracket, comma and so on. This does not make the code better. It took me 2 weeks to understand a bit of it (still not all). There are still some TODOs in it. It is just for testing. And I tested the with the new cmake. So sorry for that. I would not add my code without let someone review it, who really understands all of it. Unfortunately fnobis and brainhunter do not react any more.

[hmueller01]

Maybe we should make a new branch and call it somewhat like "P300 patch" and let it confirm from more users instead of patching the master right now. Can I have write access to push changes there at once? I will undo most of the formatting changes to concentrate on the P300 modifications.

[l3u]

What about the following: Now that cmake is on master, I create a branch with a code cleanup rework to remove that obscure formatting and unify it to some reasonable form (as proposed in https://github.com/openv/vcontrold/issues/1 ). If nobody has an objection against it, we merge the clean-up rework to master.

Then, with this new clean base, we keep on patching our stuff without the need of big formatting changes.

[hmueller01]

Ok, I have just found out that fnobis changed the r107 code for the P300 fix in a much more elegant way compared to my approach. He refered a guy named hr_17, which is not me. But I think he just misspelled, as a few fragments of the code are from me. Unfortunately it is still buggy.

vclient -h localhost:3002 -c "setGwgBetriebsart_M2 1"

sets 0, while

vclient -h localhost:3002 -c "setGwgBetriebsart_M2 Nur WW"

sets the correct value 1. All this worked correct with my solution. If I find time I will look a little bit deeper into this. In case I find the source of the problem and good solution I will make a new pull request. Would be happy making this on a new cleaned up code, as it reads more easily. :)

[hmueller01]

Where can we discuss about the formatting? This pull request is definitely not the right place ...

[l3u]

The formatting rework has now been merged to master. Probably, you want to rebase your patch to this now :-)

[hmueller01]

Thanks. I am working on this already. But it's not that easy ...

[hmueller01]

@vitoopen Did you do some coding in framer.c or was it the work of Frank Nobis?

[hmueller01]

I close this pull request as it is not needed that way. Opened a new issue to discuss the bug #13.