gijzelaerr / python-snap7

A Python wrapper for the snap7 PLC communication library
http://python-snap7.readthedocs.org/
MIT License
643 stars 245 forks source link

Fix utils.get_time() for small negative values #314

Closed LoicGRENON closed 2 years ago

LoicGRENON commented 2 years ago

I noticed utils.get_time() doesn't work for small (below one day) negative values. For example T#-1s returns 0:0:0:1.0.

With this fix it will returns -0:0:0:1.0.

BTW, i'm wondering if it wouldn't be better to return timedelta (and datetime object for all others time function as get_dt(), etc ... Or at least to let the user the choice to get a string or datetime object, maybe with a function parameter. What you think ?

swamper123 commented 2 years ago

Is that caused by the value of the PLC or the wrong handling of the get_time() in general? I haven't got a device atm to test it. EDIT: Adding a parameter, with string as default and datetime as optional, would be okay in my mind.

swamper123 commented 2 years ago

We may need more test values for these kind of functions (extreme small (not negative), everything zero ...).

gijzelaerr commented 2 years ago

i agree with the need for more tests to make sure this works properly.

LoicGRENON commented 2 years ago

I added the missing !s to minutes (its currently missing on the current branch as well).

I'm currently doing my tests using a 1513-1 CPU. I tested with T#0ms, T#1ms, T#1s, T#1m, T#1h, T#1d and their respective negative values. I could write a test for this function as well.

LoicGRENON commented 2 years ago

I can modify test.unit.py as this :

    def test_get_time(self):
        test_values = [
            (0, '0:0:0:0.0'),
            (1, '0:0:0:0.1'),  # T#1MS
            (1000, '0:0:0:1.0'),  # T#1S
            (60000, '0:0:1:0.0'),  # T#1M
            (3600000, '0:1:0:0.0'),  # T#1H
            (86400000, '1:0:0:0.0'),  # T#1D
            (2147483647, '24:20:31:23.647'),  # max range
            (-0, '0:0:0:0.0'),
            (-1, '-0:0:0:0.1'),  # T#-1MS
            (-1000, '-0:0:0:1.0'),  # T#-1S
            (-60000, '-0:0:1:0.0'),  # T#-1M
            (-3600000, '-0:1:0:0.0'),  # T#-1H
            (-86400000, '-1:0:0:0.0'),  # T#-1D
            (-2147483647, '-24:20:31:23.647'),  # min range
        ]

        data = bytearray(4)
        for value_to_test, expected_value in test_values:
            data[:] = struct.pack(">i", value_to_test)
            self.assertEqual(snap7.util.get_time(data, 0), expected_value)

What you think ?

swamper123 commented 2 years ago

That modification would be great.