modification of TIME data type #311
Conversation
snap7/util.py
Outdated
| >>> snap7.util.set_dint(data, 0, '-22:3:57:28.192') | ||
|
|
||
| >>> snap7.util.set_time(data, 0, '-22:3:57:28.192') | ||
|
|
||
| >>> data | ||
| bytearray(b'\x8d\xda\xaf\x00') | ||
| """ | ||
| import re |
There was a problem hiding this comment.
This import statement is still not needed, since it is already imported at the top of the file.
|
Could the errors from Mypy be ignored? :) |
|
@gijzelaerr Could you please merge the new version to master? Thank you! |
snap7/util.py
Outdated
| minutes: int = int(data_list[2]) | ||
| seconds: int = int(data_list[3]) | ||
| milli_seconds: int = int(data_list[4]) | ||
| if re.split(r'(\d+)$', days)[0:2][0] == '-': |
There was a problem hiding this comment.
The same, but a little more readable, imho.
| if re.split(r'(\d+)$', days)[0:2][0] == '-': | |
| if re.match('^-\d{1,2}$', days): |
snap7/util.py
Outdated
| if abs(int(days)) <= 24 and hours <= 23 and minutes <= 59 and seconds <= 59 and milli_seconds <= 999: | ||
| if int(days) * sign == 24 and hours > 20 and minutes > 31 and seconds > 23 and milli_seconds > 647 or \ | ||
| int(days) * sign == -24 and hours > 20 and minutes > 31 and seconds > 23 and milli_seconds > 648: | ||
| raise ValueError('time value out of range, please check the value interval') | ||
|
|
||
| time_int = ((int(days) * sign * 3600 * 24 + (hours % 24) * 3600 + (minutes % 60) * 60 + seconds % 60) * 1000 + milli_seconds) * sign |
There was a problem hiding this comment.
This is a good bulletproof check, but this code is not easy to read (at least for me). It may be better to calculate the result first and then check the range. It is less efficient, but more readable.
| if abs(int(days)) <= 24 and hours <= 23 and minutes <= 59 and seconds <= 59 and milli_seconds <= 999: | |
| if int(days) * sign == 24 and hours > 20 and minutes > 31 and seconds > 23 and milli_seconds > 647 or \ | |
| int(days) * sign == -24 and hours > 20 and minutes > 31 and seconds > 23 and milli_seconds > 648: | |
| raise ValueError('time value out of range, please check the value interval') | |
| time_int = ((int(days) * sign * 3600 * 24 + (hours % 24) * 3600 + (minutes % 60) * 60 + seconds % 60) * 1000 + milli_seconds) * sign | |
| time_int = ((int(days) * sign * 3600 * 24 + (hours % 24) * 3600 + (minutes % 60) * 60 + seconds % 60) * 1000 + milli_seconds) * sign | |
| if time_int < -2147483648 or time_int > 2147483647: | |
| raise ValueError('time value out of range, please check the value interval') |
There was a problem hiding this comment.
This wouldn't solve problems like "negative seconds".
time_string must match with a strict pattern, before stuff is splitted. Otherwise stuff will break.
There could be other advantages, if the string was checked properly first, too.
There was a problem hiding this comment.
@swamper123 Yes, as I wrote this function, I also thought, in which format the time_string should be.
I just use the format as the return value from get_time().
There was a problem hiding this comment.
@Yingliangzhe The format is totally fine and I would have made the same way, if machines return it like that.
But unfortunatly, the Dr. Evils and "DAU"s (like germans say 😉 ) like to break things. So inputs should be bulletproof. ^^
There was a problem hiding this comment.
@nikteliy this piece of code, you suggested, cannot cover the case, if I write '-24:25:30:23:193' to time_string.
As intermediate result of time_int, it would be -2079023193 and still inside of the interval [-2147483648, 2147483647], but this time format should not be acceptable.
There was a problem hiding this comment.
Yup, my mistake. I give up then :)
There was a problem hiding this comment.
re.fullmatch(r"-?([2][0-4]|[1]\d|\d):([2][0-3]|[1]\d|\d):([1-5]\d|\d):([1-5]\d|\d):\d{1,3}.\d{1,3}", time_str)
This should check all legit patterns (I needed a while to get in), but not if min/max is reached (because if the day is 24, hours shouldn't be greater than 20 .... well it's my first thought, but I got an idea. ^^
There was a problem hiding this comment.
@Yingliangzhe Could you verify if this is legit, even with min/max values?
if re.fullmatch(r"(-?(2[0-3]|1?\d):(2[0-3]|1?\d|\d):([1-5]?\d):[1-5]?\d.\d{1,3})|"
r"(-24:(20|1?\d):(3[0-1]|[0-2]?\d):(2[0-3]|1?\d).(64[0-7]|6[0-3]\d|[0-5]\d{1,2}))|"
r"(24:(20|1?\d):(3[0-1]|[0-2]?\d):(2[0-3]|1?\d).(64[0-8]|6[0-3]\d|[0-5]\d{1,2}))"There was a problem hiding this comment.
To clear this thing a bit up, each line is a case (normal, min, max):
- (-)0-23 days : 0-23hours : 0-59minutes : 0-59seconds . 0-999 miliseconds
- -24 days : 0-20hours: 0-31 minutes: 0-23 seconds. 0-647 milliseconds
- 24 days : 0-20hours: 0-31 minutes: 0-23 seconds. 0-648 milliseconds
(at least I hope it matches, which would be great)
There was a problem hiding this comment.
Hello @swamper123,
sorry for the late answer, I have checked the solution you suggested.
I don't think, it has no problem by interpreting the TIME data type.
But, as far as I remember, the max value of TIME, the milliseconds should be 0 - 647, and the min value should be 0 - 648. I have changed the corresponding position.
snap7/util.py
Outdated
| if sign < 0: | ||
| time_int = (1 << bits) + time_int | ||
| bytes_array = time_int.to_bytes(4, byteorder='big') |
There was a problem hiding this comment.
The sign check can be replaced by passing a parameter signed=True to the method .to_bytes().
| if sign < 0: | |
| time_int = (1 << bits) + time_int | |
| bytes_array = time_int.to_bytes(4, byteorder='big') | |
| bytes_array = time_int.to_bytes(4, byteorder='big', signed=True) |
Co-authored-by: nikteliy <52915342+nikteliy@users.noreply.github.com>
|
Hello @gijzelaerr, |
I have modified the code little bit and added some tests to solve #310