Skip to content

Conversation

@Novecento99
Copy link
Contributor

@Novecento99 Novecento99 commented Sep 10, 2024

Hi,
as anticipated I was working on a function that could ingest a native TIA portal export of a non-optimized db to generate a string compatible with the DB class layout_specification

It also manage names duplicates by adding a suffix "_X" at the end of the variables, where "X" is a progressive numeration.

snap7/util/db.py Outdated
with open(txt_path, "r") as file:
db_specification = ""

valid_list = ["BOOL", "DWORD", "INT", "DINT", "CHAR", "STRING", "DATE_AND_TIME", "TIME_OF_DAY", "REAL", "BYTE"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: If you replace the list with a set, the operation x in s will be faster

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

parsed_line = line.split("\t")

var_name = parsed_line[0]
var_type = parsed_line[1].upper()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why the literals in valid_names are uppercase? If they were lowercase, you wouldn't need to call .upper() for every line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valid_names are list a copied from another part of this repo.
TIA portal exports variabiles types with the first letter in uppercase anyway


var_name = parsed_line[0]
var_type = parsed_line[1].upper()
var_offset = parsed_line[2]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what the export file format looks like. Can there be empty lines or comments in it? This could cause an exception, something like 'list index out of range'

Copy link
Contributor Author

@Novecento99 Novecento99 Sep 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I know: no.
this is an example of tia export:

PRESENT Bool 3968.0 false --- False True True True False PIECE PRESENT SCRAP Bool 3968.1 false --- False True True True False PIECE SCRAP READY Bool 3968.2 false --- False True True True False PIECE READY DRY_TEST Bool 3968.3 false --- False True True True False DRY TEST MOVING Bool 3968.4 false --- False True True True False PIECE MOVING SCANNED Bool 3968.5 false --- False True True True False QR_NOK Bool 3968.6 false --- False True True True False QR CODE NOT OK LONG_PROFILE Bool 3968.7 false --- False True True True False VB_ACK Bool 3969.0 false --- False True True True False

snap7/util/db.py Outdated
var_name = var_name + to_add
var_names.append(var_name)

if var_type:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If var_type is an empty string, then var_type in valid_list is will be False. So there is no point to check var_type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay thankyou

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

var_offset = parsed_line[2]

to_add = "_0"
for name in reversed(var_names):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using a dictionary, where the key is var_name and the value is a counter, would be cleaner and more efficient than using a list. Something like this:

var_names: dict[str, int] = {}
to_add = var_names.setdefault(var_name, 0)
var_names[var_name] += 1
var_name = f'{var_name}_{to_add}'

And you won’t need an inner loop in this case.

snap7/util/db.py Outdated
if var_type:
if var_type in valid_list:
new_line = var_offset + "\t" + var_name + "\t" + var_type
db_specification = db_specification + "\n" + new_line
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Perhaps appending "new_line" to a list and then using join() is more efficient. But I’m not sure it makes a significant difference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt so, especially considering this function is meant to be run only one time for db....

@lupaulus
Copy link
Contributor

@Novecento99 Nice work

@gijzelaerr
Copy link
Owner

Code Review

Thanks for contributing this TIA Portal export parser! This is a useful feature. However, there are several issues that need to be addressed before we can merge.

1. Bug: Duplicate name detection is broken for underscored variables

The current logic uses rsplit("_")[0] which fails for variable names containing underscores:

>>> "my_value_0".rsplit("_")
['my', 'value', '0']
>>> "my_value_0".rsplit("_")[0]
'my'  # Expected: 'my_value'

This means duplicates of my_value will all get _0 suffix instead of _0, _1, _2:

# Current behavior:
'my_value' -> 'my_value_0'
'my_value' -> 'my_value_0'  # BUG: should be 'my_value_1'

Fix: Use rsplit("_", 1) to split only on the last underscore:

if name.rsplit("_", 1)[0] == var_name:
    to_add = "_" + str(int(name.rsplit("_", 1)[-1]) + 1)

2. Remove debug print statement

Line with print(name.rsplit("_")[-1]) should be removed or replaced with logger.debug().

3. Docstring parameter mismatch

The docstring says tia_export but the parameter is named txt_path.

4. Missing error handling

What happens if:

  • A line doesn't have 3 tab-separated fields?
  • The file doesn't exist?
  • The offset field isn't valid?

Consider adding try/except or validation.

5. Incomplete type list

The valid_list is missing types that are supported elsewhere in the codebase:

  • WORD, UINT, UDINT, LREAL, SINT, USINT, TIME, DATE

Consider matching the types supported in Row.get_value() and Row.set_value().

6. Inefficient string concatenation

Using db_specification = db_specification + "\n" + new_line in a loop is O(n²). Use a list instead:

lines = []
for line in file:
    ...
    if var_type in valid_list:
        lines.append(f"{var_offset}\t{var_name}\t{var_type}")
return "\n".join(lines)

7. No tests

Please add unit tests. At minimum:

  • Test with a sample TIA export file
  • Test duplicate name handling (including names with underscores)
  • Test with missing/invalid fields

8. Separate formatting changes

The PR includes many unrelated line-wrapping changes. Please split those into a separate PR or remove them to keep this PR focused.


Summary of required changes:

  1. Fix the rsplit bug (use rsplit("_", 1))
  2. Remove print() statement
  3. Fix docstring
  4. Add basic error handling
  5. Add missing types to valid_list
  6. Use list + join instead of string concatenation
  7. Add unit tests
  8. Remove unrelated formatting changes

Happy to help if you have questions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants