Added support for updating children units symbol ID

[eeintech]

Tested with Ki-nTree and works fine, happy to hear what do you think @mvnmgrx

[mvnmgrx]

Thanks for your contribution!

I haven't had the time to fully go through your code, but here are some initial thoughts:

At the moment i am uncertain if dealing with a subsymbol's unit or demorgan-ID is the responsibility kiutils, or the application that implements kiutils. The documentation calls this field LIBRARY_ID/UNIT_ID and, depending on the context the symbol is in, it will be interpreted differently by KiCad (see Symbol Unit Identifier and Library Identifier).

Parsing the LIBRARY_ID/UNIT_ID with just a simple split() may cause some problems:

• For some reason, it removes parts that are divided by a : (check the unittests for this)
• What happens if the subsymbol ID contains more than 2 underscores?
• Do quoted strings still work then?
• ...

These are all test cases that need to be considered. Parsing of the LIBRARY_ID/UNIT_ID field could be done more reliable with some regexes of some sort ..

If this should be implemented, i would rather see two new attributes for the Symbol() class:

• unitId: int = None: The unit identifier
• styleId: int = None or bodyStyleId: int = None: The body style this subsymbol represents

Both fields None means that the symbol is a top level symbol. Both values set means that the symbol is an embedded symbol. The ..._0_1 part is then automatically added in Symbol.to_sexpr() if neccessary.

The application implementing kiutils could then set the same id: str = "" for the top-level symbol as well as its subsymbols, removing the need to pass around the parent_id parameter.

This should be more consistent to the KiCad documentation.

Would such an approach fit your use-case? Happy to hear your thoughts on this.

[eeintech]

Hello @mvnmgrx

Thanks for taking the time to review this PR!

At the moment i am uncertain if dealing with a subsymbol's unit or demorgan-ID is the responsibility kiutils

I would counter argue that providing a mean to go from s-expr to python dataset means that the python dataset can be modified then exported back to s-expr. If kitutils can't handle python dataset changes and is a one-way parser (eg. s-expr to python dataset) then you should simply remove the to_sexpr() methods from all classes 😄 It's a bit extreme but that's where my mind goes when I see this method it means kitutils "knows" the python dataset could have been modified and still support the conversion back to s-expr. I don't see anybody doing s-expr->python->s-expr for the sake of it, not changing anything on the way!

• For some reason, it removes parts that are divided by a : (check the unittests for this)

I was not aware of CI errors until now because I need your approval for running it 😉

These are all test cases that need to be considered. Parsing of the LIBRARY_ID/UNIT_ID field could be done more reliable with some regexes of some sort ..

Piggybacking on "KiCad Library Utils" re.match method seems to work well, check out the updated code.

• unitId: int = None: The unit identifier
• styleId: int = None or bodyStyleId: int = None: The body style this subsymbol represents

Also added, I agree it's better to stick to official doc, in this case I thought it would be more efficient to do it the way I did but meh no real reason to do so.

[mvnmgrx]

Hello, thanks for incorporating this change. I marked some minor things in the code and i would be happy to merge your PR once fixed.

I would counter argue that providing a mean to go from s-expr to python dataset means that the python dataset can be modified then exported back to s-expr.

kiutils already supplies its application with an interface to the bare LIBRARY_ID token in the form of the Library.id class member, so converting both ways while modifying is already possible, yet pretty cumbersome for the application as KiCad unfortunately encodes multiple things in this token.. I agree that it would make more sense to split this up further into its parts.

I was not aware of CI errors until now because I need your approval for running it 😉

No problem ;-) You can also run the unit tests on your machine using python3 -m unittest in the repository root.

One other thing to discuss: You have decided to pass a subsymbol's parent name via the parent_id parameter. I guess this is okay, but it overrides everything set to the subsymbol's id token, which may be ambiguous. The documentation states, that a subsymbol's "name" (thus its Symbol.id field) is always the parent name. Wouldn't it makes more sense to do the following:

• Always set Symbol.id to a symbol's name without the _X_X part (regardless if it is a top-level symbol a subsymbol)
• Add unitId and styleId everytime they are not None when calling Symbol.to_sexpr()

This would remove the parent_id parameter completely and create a generalized Symbol class (if i am not overlooking something right now). Some implications for the application that come to my mind:

• Setting unitId and styleId for a top-level symbol is invalid
• When renaming a symbol, all of its subsymbols have to be renamed as well

Looking forward to hear your thoughts on this and if such an approach would be okay for your use-case as well.

PS: We also need unit tests for your implementation, but i can do that as well when your PR is merged

Best regards

[mvnmgrx]

The rabbit hole goes deeper...

The unittests are failing because because the regex does not work .. Furthermore i noticed that a top level symbol may have a LIBRARY_NICKNAME in its ID field that is separated by a : (colon) (refer to here). A subsymbol of said top level symbol does not have this LIBRARY_NICKNAME encoded in its ID field ..

Traceback of one test that is failing:

ERROR: test_addPropertyToSchematicSymbol (tests.test_schematic.Tests_Schematic.test_addPropertyToSchematicSymbol)
Adds a new property to an already existing symbol in the schematic and verifies the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\etc\py-eeintech-kiutils\tests\test_schematic.py", line 31, in test_addPropertyToSchematicSymbol
    schematic = Schematic().from_file(self.testData.pathToTestFile)
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\etc\py-eeintech-kiutils\src\kiutils\schematic.py", line 169, in from_file
    item = cls.from_sexpr(sexpr.parse_sexp(infile.read()))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\etc\py-eeintech-kiutils\src\kiutils\schematic.py", line 129, in from_sexpr
    object.libSymbols.append(Symbol().from_sexpr(symbol))
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\etc\py-eeintech-kiutils\src\kiutils\symbol.py", line 297, in from_sexpr
    raise Exception(f'Failed to parse symbol unit identifiers due to invalid format: {symbol_unit.id}')
Exception: Failed to parse symbol unit identifiers due to invalid format: 1.5KExxCA_0_1

When i change this ..

symbol_id_parse = re.match(r"^" + re.escape(object.id) + r"_(\d+?)_(\d+?)$", symbol_unit.id)

.. to this ..

symbol_id_parse = re.search(r"_(\d+?)_(\d+?)$", symbol_unit.id)

.. the tests ran but the output is not correct now.

Comparing parts of this tests input:

# ...
      (symbol "1.5KExxCA_1_1"
        (pin passive line (at -3.81 0 0) (length 2.54)
          (name "A1" (effects (font (size 1.27 1.27))))
# ...

to its output:

# ...
      (symbol "Diode:1.5KExxCA_1_1"
        (pin passive line (at -3.81 0 0) (length 2.54)
          (name "A1" (effects (font (size 1.27 1.27))))
# ...

Subsymbols now also have this LIBRARY_NICKNAME field in their ID, which is not correct. So we need to add another token to the Symbol class to stay consistent with your changes, which also needs to be parsed correctly.

How about Symbol.libraryNickname which takes the part before the : (colon) ?

[eeintech]

Hello @mvnmgrx

Thanks for your thorough feedback, unfortunately I'm currently in vacation for the next couple of weeks... But I will definitely work on this when I come back, sorry for the delay!

[eeintech]

Hello @mvnmgrx

Ok so in addition to your earlier review, here are the latest changes:

• Added both libraryNickname and entryName parameters as optional strings (reference: https://dev-docs.kicad.org/en/file-formats/sexpr-intro/index.html#_library_identifier)
• Added parentSymbolName as optional string
• Updated id parameter to class property
• Added setter method to id parameter
  • Attempt to find libraryNickname and entryName values with RegEx match on the symbol ID
  • Update Value property value with new ID value
  • For each symbol units, set its parentSymbolName to either the parent entryName if it exists or parent ID. This allows overwriting of the Symbol ID and automatic update of units naming
• Run Unit and Style IDs RegEx match using parentSymbolName instead
• In to_sexpr remove the parent_id parameter and use the parentSymbolName instead to construct the symbol units IDs

I ran my tests and yours and everything works fine on my side. Let me know what you think, thanks!

[mvnmgrx]

Thanks for incorporating the changes! Hope you had a nice vacation as well.

I've added some review notes, but read this comment first:

The problem is that self._id essentially always has to have the same information in it as in libraryNickname, entryName, unitId, styleId and parentSymbolName combined. When setting any of the former attributes in the application and then reading from self.id, the wrong library identifier will be returned.

So it would be nicer to remove the self._id token completely and to use the getter/setter of id to always generate the correct identifier based of those other tokens.

Something like this:

    @property
    def id(self):
        unit_style_ids = f"_{self.unitId}_{self.styleId}" if (self.unitId and self.styleId) else ""
        if self.libraryNickname:
            return f'{self.libraryNickname}:{self.entryName}{unit_style_ids}'
        else:
            return f'{self.entryName}{unit_style_ids}'

    @id.setter
    def id(self, symbol_id):
        # Split library id into nickname, entry name, unit id and style id (if any)
        # Update the value property for parent-level symbols (keep in mind that this is a side-effect of this setter and needs to be documented!)
        # Update child unit's id/name/etc (this may also be a side-effect, but is neccessary to keep consistency)

This simplifies a lot in to_sexpr(), as assigning object.id = exp[1] should take care of the rest, and reading of this token will consistently produce the correct format for the library identifier. This should work for parent and child symbols the same way (correct me if i'm wrong here).

This would also remove parentSymbolName. This token is redundant, as a child symbol always resides inside a top-level symbol when parsing valid symbols. So the application already "knows" the parent symbol for each child symbol. The same would be true when generating symbols from scratch. As far as i know, "deeply nested symbols" or somesuch are not a thing.

My goal is to keep the implementation of the Symbol() class the same, regardless if we are dealing with a parent symbol or with a child symbol.

[mvnmgrx]

Btw. i noticed that the SchematicSymbol() class also uses such a combined identifier. This may also be subject to change some time later if needed.

[eeintech]

All changes were implemented, please have a look at the new id.setter method and let me know what do you think.

I am not sure what do you mean by documenting that the Value property and units ID being updated automatically when main symbol ID changes, this is a behavior straight from KiCad so it mirrors it.

[mvnmgrx]

Nice work, thanks! I annotated two minor things in the review.

Also some more thoughts on the following:

I am not sure what do you mean by documenting that the Value property and units ID being updated automatically when main > symbol ID changes, this is a behavior straight from KiCad so it mirrors it.

This may be the way KiCad does it in the library editor by default. But in the following example it would be unexpected behavior:

• Take some generic part into your schematic (e.g. a resistor into your schematic, library ID "Device.R")
• Modify its value to something other (e.g "R" to "120k")
• The library ID stays the same, but the "Value" property changed

The other way around (changing the library ID and the Value stays the same) would also be possible in the schematic editor (using the "Change Symbol" button in its properties).

Furthermore, setting the Value property and the library ID differently produces valid S-Expression that can be opened in the symbol editor.

From this i would propose that the entryName token and the Value property shouldn't be automatically changed together. You can still do it in your application code, if you need both to be the same. But always changing both is at least a side-effect of the id.setter and i would personally argue that it is unexpected behavior.

[mvnmgrx]

But this should be it. Then we have a nice consistent API for reading and writing both the raw library ID as well as its components. Looking forward to merging your PR after those changes!

I'm going to write some more unittests after the merge to cover every part of the renaming as well as the regex parsing a bit more in detail. Should be done with that in the next few days, so it will be available in v1.2.2 asap.

[eeintech]

@mvnmgrx I totally understand for the Value now, I was only thinking symbol libraries and not schematics.
I removed the automatic update as requested.

Please check one last time on your side, I added a couple of tests for merging two libraries together and renaming a symbol. I have done some RegEx tests on the side to verify the logic, it checks out from what I could see.

Thanks for taking so much of your time following up and helping out, really appreciated!

[mvnmgrx]

Thanks for taking your time to contribute as well!

[mvnmgrx]

FYI: I've renamed the id token to libId to be consistent with the changes in #54

Another thing that i changed was to simplify the parsing in Symbol.id, as the first regex is now only used to check for invalid formats of this token. I also added one more unittest for setting and unsetting all variations of this token. It should work now as expected, but you may have to rename the id token if you are using this API in your scripts.

[eeintech]

@mvnmgrx Sure thing I can test your changes. Are you at the point where you will cut the v1.2.2 release soon? I would like to add it to Ki-nTree as a pip package, thanks!

[mvnmgrx]

Pretty soon, I'm planning on today evening for now (european time). It will furthermore be named v1.3.0 instead as they were too many breaking changes to still call this a "minor" update.

[eeintech]

Awesome, thanks for the update 👍

[mvnmgrx]

FYI: https://github.com/mvnmgrx/kiutils/releases/tag/v1.3.0

[eeintech]

Looks good on my side 👍