rscada / libmbus

Meter-bus library and utility programs
http://www.rscada.se/libmbus
BSD 3-Clause "New" or "Revised" License
224 stars 138 forks source link

WIP/RequestForFeedback/Discussion PR to add MSVC build capability #133

Open Apollon77 opened 6 years ago

Apollon77 commented 6 years ago

Hi,

this PR is not really for "accepting" but to start a feedback/discussion/enhancement round :-) I hope this is ok.

While working on my nodejs native module for libmbus (https://github.com/Apollon77/node-mbus) I also started liiking into windows build capability and I managed to get it build on windows (currently the lib only, but adding the executables should be no big deal if someone knows how :-))

I did the following (and those are the changes in this PR):

With these changes I was able to work with Serial and TCP on Windows too and I use this branch in my nodejs native module already.

It would be great to get feedback from you on this ... Especially about what your opinion if for Win support and such :-)

Ingo

Apollon77 commented 6 years ago

@lategoodbye ... Any chance that you have a look on this approach "in general" and give some feedback? I mainly need to know if you have an interest in this or not, and if what your needs would be

lategoodbye commented 6 years ago

In general i'm okay with Windows support, but here are some points:

Apollon77 commented 6 years ago

Thank you for the check and your feedback. Very important for me.

no regressions (please verify that Linux still works)

For my nodejs module I have a full test suite with simulated TCP and virtual serialports for Linux, MacOS and Windows - because in the background libmbus is used this is a gread test to see that still anything works. With some support on Linux side the test suite could easiely be adopted also to run the tests after each build for libmbus directly. If you are interested ... Here some links that show what the tests do: Linux/Macos (using socat for serial tests): https://travis-ci.org/Apollon77/node-mbus/builds/353718420 Windows (using com0com Virtual Nullmodel Simulator): https://ci.appveyor.com/project/Apollon77/node-mbus Works very great when you know protocol level.

keep code maintainable (no ifdef voodoo, try to move all windows specific stuff into a separate module)

Also thought about that. It would mainly be then an own "mbus-serial-win.c" and "mbus-tcp-win.c" then included using still #ifdefs but not that many. Only pitfall here is that you have duplicated code in those to files then. Could be acceptable, but it is redundant stuff in many places (especially with tcp). I was not sure if this is really the "best" way. or how you would think on "multiple modules"? Else on top of the files some "ifdef" magic needs to happen because you need to include dufferent header files on windows vs linux/mac.

don't introduce GPL code

You are right. Oversee that. Need to think (also with the "different module" topic above how is the best way to go.

in case you need a new build system like cmake this should be introduced first as a separate pull request

This was just a try, so not relevant mor me anymore ... msvc on windows (unless someone add more)

try to collect some thumbs up

Already tried by referencing thjis in all "windows related" issues ... but noone reacted ... so this is hard for me at all ...

And to be really honest: I use this includng WIndows in my nodejs module and will continue that with my own branch/fork also if Windows support is not added officially to libmbus. I will hold it up to date ... But will try to do anythink from my side to get it added ...

I will only need some time to refactor things. I would like to leave this PR open till I are done with that and then will replace it.

PS: Would you accept the timeout changes as own PR because of "proofed right in field tests"?

lategoodbye commented 6 years ago

M-Bus is very timing critical. Beside your Travis test, do you test scanning of real M-Bus devices?

I'm not sure which timeout changes do you mean. Without a proper commit message this is hard to guess and identify. Maybe 06cdd2788ee9f09e0416021f72869cb291703823? In this case it won't be acceptable because such big timeouts are beyond the M-Bus spec and i merge all serial users would complain about this long timeout. I think you better explain your special setup.

Apollon77 commented 6 years ago

M-Bus is very timing critical. Beside your Travis test, do you test scanning of real M-Bus devices?

Yes I also test a device with an USB Master Dongle at home on Linux and Windows. Worked and works. And I also verified that CI systems had same results on different code-versions then the "real life" test ... so the CI stuff shown in general that things work logically. And yes real tests are done additional

I'm not sure which timeout changes do you mean. Without a proper commit message this is hard to guess and identify. Maybe 06cdd27? In this case it won't be acceptable because such big timeouts are beyond the M-Bus spec and i merge all serial users would complain about this long timeout. I think you better explain your special setup.

I for myself do not had problems till now. I got an report from a user and also found then other experiences while browsong through the commits in forks from your limbmbus, so it seems to be a topic that can happen ... But yes I can also try to reduce them again to default and see other complains come in

Apollon77 commented 6 years ago

aahhh yes :-)) Thank you!! Just a try if it prevents a segfault one user had when calling "close" after a manual disconnect of the USB device

Apollon77 commented 6 years ago

@lategoodbye DO you had a chance to think about:

keep code maintainable (no ifdef voodoo, try to move all windows specific stuff into a separate module)

Also thought about that. It would mainly be then an own "mbus-serial-win.c" and "mbus-tcp-win.c" then included using still #ifdefs but not that many. Only pitfall here is that you have duplicated code in those to files then. Could be acceptable, but it is redundant stuff in many places (especially with tcp). I was not sure if this is really the "best" way. or how you would think on "multiple modules"? Else on top of the files some "ifdef" magic needs to happen because you need to include dufferent header files on windows vs linux/mac.

lategoodbye commented 6 years ago

I think platform specific communication modules would be okay.

sympthom commented 6 years ago

Great work on the port to Windows! Fingers crossed. :)

Apollon77 commented 6 years ago

Thank you! According to the feedback above I need to rework several things ... need to see when I find time for that :-(