dbc: dump_string: set GenSigStartValue from Signal.raw_initial

[malsyned]

I modeled this after the way GenMsgCycleTime is synchronized with Message.cycle_time.

[coveralls]

Pull Request Test Coverage Report for Build 5064782045

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 16 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.002%) to 93.856%

---

Files with Coverage Reduction	New Missed Lines	%
cantools/database/can/formats/dbc.py	16	98.22%

Totals
Change from base Build 5027870252:	-0.002%
Covered Lines:	7241
Relevant Lines:	7715

---

💛 - Coveralls

[andlaus]

LGTM. @juleq , @zariiii9003 ?

[zariiii9003]

You are modifying signal.dbc.attributes here, because you reassigned sig_attributes on line 802. You should call sig_attributes.update(signal.dbc.attributes) or create a deep copy of signal.dbc.attributes.

[malsyned]

I wondered about that, but if that's a bug, it also exists the way message.dbc.attributes['GenMsgCycleTime'] is handled in the current code right above my additions:

    for message in database.messages:
        # retrieve the ordered dictionary of message attributes
        msg_attributes = OrderedDict()
        if message.dbc is not None and message.dbc.attributes is not None:
            msg_attributes = message.dbc.attributes

        # synchronize the attribute for the message cycle time with
        # the cycle time specified by the message object
        if message.cycle_time is None and 'GenMsgCycleTime' in msg_attributes:
            del msg_attributes['GenMsgCycleTime']

That was originally added in fcb2bcb. @andlaus can you comment on what your thinking was there? It looks to me like you were taking the opportunity at DBC write time to synchronize the Message DbcSpecifics with any changes made to the Message.cycle_time value. Since what I'm doing is an analogous operation on Signals, I used the same pattern.

If you still think that's wrong, though, @zariiii9003, I can fix it. Would you want it fixed for message.dbc.attributes['GenMsgCycleTime'] as well?

[andlaus]

That was originally added in fcb2bcb. @andlaus can you comment on what your thinking was there? It looks to me like you were taking the opportunity at DBC write time to synchronize the Message DbcSpecifics with any changes made to the Message.cycle_time value.

I don't remember exactly, but I think that was the reason why. I'm pretty sure this can be implemented more elegantly, though.

Since what I'm doing is an analogous operation on Signals, I used the same pattern.

fine with me ;). if I understood @zariiii9003 correctly, he just wants to have this done in a more pythonic fashion?

[andlaus]

You are modifying signal.dbc.attributes here, because you reassigned sig_attributes on line 802. You should call sig_attributes.update(signal.dbc.attributes) or create a deep copy of signal.dbc.attributes.

NB that the whole database is already deepcopied before it is written: https://github.com/cantools/cantools/blob/master/cantools/database/can/formats/dbc.py#L1862

[zariiii9003]

In my opinion, dumping the database should have no side effects. The database should remain unchanged.

[andlaus]

In my opinion, dumping the database should have no side effects. The database should remain unchanged.

agreed. that's why the database object is deepcopied before doing the modifications. (It's a bit hacky but it has been like this since at least 2020 (cf 89b974cb3b0921a057bff352afe84fa844e23a8d)

[malsyned]

Given that the deepcopy already happens at the top (I'd missed that -- thanks @andlaus) I think your request is already met by this PR in its current form, do you agree @zariiii9003? Does it need anything else before you'd want to merge it?

[zariiii9003]

In my opinion, dumping the database should have no side effects. The database should remain unchanged.

agreed. that's why the database object is deepcopied before doing the modifications. (It's a bit hacky but it has been like this since at least 2020 (cf 89b974cb3b0921a057bff352afe84fa844e23a8d)

I see. That module needs some refactoring 😄

[andlaus]

agreed. it is far from the only one, though ;)