dmroeder / pylogix

Read/Write data from Allen Bradley Compact/Control Logix PLC's
Apache License 2.0
599 stars 182 forks source link

'else' seems missing in eip.py line ~511 #129

Closed David-2D3FE9 closed 4 years ago

David-2D3FE9 commented 4 years ago

Hello all, The raw=True option seems doesn't work in eip.py => def _getPLCTime(self, raw=False)

near line 511 we have:

        if status == 0:
            # get the time from the packet
            plc_time = unpack_from('<Q', ret_data, 56)[0]
            if raw:
                value = plc_time
            human_time = datetime(1970, 1, 1) + timedelta(microseconds=plc_time)
            value = human_time
        else:
            value = None

I think we should have

        if status == 0:
            # get the time from the packet
            plc_time = unpack_from('<Q', ret_data, 56)[0]
            if raw:
                value = plc_time
            else:  # <=== here ;-)
                human_time = datetime(1970, 1, 1) + timedelta(microseconds=plc_time)
                value = human_time
        else:
            value = None

I used this code today to synchronise my raspberrypi from PLC time I added the 'else' and was able to switch between raw and human values

Sorry in advance if i'm wrong for some reason And thank's a lot for all this impressive work !

TheFern2 commented 4 years ago

Hi @Himelsasuki this seems like a legitimate bug, thanks for reporting it. Seems like not many people use raw time. : )

Current code:

>>> conn = PLC("192.168.1.207")
>>> conn.GetPLCTime()
Response(TagName=None, Value=2020-05-04 20:17:24.800142, Status=Success)
>>> conn.GetPLCTime(raw=True)
Response(TagName=None, Value=2020-05-04 20:17:33.423794, Status=Success)

After adding the else you suggested:

>>> from pylogix import PLC
>>> conn = PLC("192.168.1.207")
>>> conn.GetPLCTime(raw=True)
Response(TagName=None, Value=1588623300862826, Status=Success)
>>> conn.GetPLCTime()
Response(TagName=None, Value=2020-05-04 20:15:17.065021, Status=Success)

Would you like to submit a pull request?

David-2D3FE9 commented 4 years ago

Yes i wish. I've just created my GitHub account few minutes ago for this, and this is only my 4th day of python. So let me an hour to do it correctly :D

TheFern2 commented 4 years ago

No problem, take your time. We probably should add a guide on making pull requests.

git add pylogix/eip.py
git add pylogix/__init__.py
git add changelog
git comit -m "rawtime-patch-01"
git push origin rawtime-patch

You can do git add *, but you must be certain not to add unwanted files. Before adding any files, you can do git status to make sure the only new changes are eip, init, and changelog.

Then when you go on your github repo https://github.com/Himelsasuki/pylogix, you'll see a pull request popup. Hopefully this makes sense, any questions let me know.

TheFern2 commented 4 years ago

Issue fixed on #130. Thanks! @Himelsasuki

@dmroeder I suggest we do pypi releases quarterly or something along those lines, to avoid too many pypi releases, could be 4 or 6 per year. We can add in the readme that if you want to install latest updates to use the old method of install without pypi.