Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "When converting to Fibex keep the signal instance start position as in dbc file" #845

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

SabrineBH
Copy link
Contributor

Reverts #832

@SabrineBH
Copy link
Contributor Author

@ebroecker

Apologies for creating the revert PR. The original change caused a regression, and reverting seemed like the best option at the time.

Sorry for any inconvenience. Could we proceed with merging this? Let me know your thoughts. Thanks for your understanding!

@ebroecker
Copy link
Owner

Hi @SabrineBH

what happend with the previous patch applied? What "regression" happens with it?
Could you give me some side-info for it?

I tried to check, and I think the patch was correct, so reverting it may do it worse again.

Maybe we should also create some very small examples for testing the code regarding this issue

@SabrineBH
Copy link
Contributor Author

It's not exactly a regression in the traditional sense, but it is in the context of my solution.

Let me explain:

  • For example, consider the DBC file (link).

    Old behavior:

The signal

  SG_ sig0 : 1|2@0+ (1,0) [0|0] ""  CCL_TEST  

was converted to:

<fx:SIGNAL-INSTANCE ID="PDUINST_testFrame1.sig0">
    <fx:BIT-POSITION>6</fx:BIT-POSITION>
    <fx:IS-HIGH-LOW-BYTE-ORDER>true</fx:IS-HIGH-LOW-BYTE-ORDER>
    <fx:SIGNAL-REF ID-REF="SIG_testFrame1.sig0"/>
</fx:SIGNAL-INSTANCE>

As you can see, the bit position changed from 1 (in the DBC) to 6 in the converted Fibex file.

After the fix:
The same signal is now converted as follows:

<fx:SIGNAL-INSTANCE ID="PDUINST_testFrame1.sig0">
  <fx:BIT-POSITION>1</fx:BIT-POSITION>
  <fx:IS-HIGH-LOW-BYTE-ORDER>true</fx:IS-HIGH-LOW-BYTE-ORDER>
  <fx:SIGNAL-REF ID-REF="SIG_testFrame1.sig0"/>
</fx:SIGNAL-INSTANCE>

This means that the bit position remains unchanged (1), which is technically correct.

However, this change impacted my solution, where I present DBC data in a tree list and order signals based on bit transmission order. Because of this, the order of signals changed, leading to incorrect encoding/decoding.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 13542587760

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 54.518%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/canmatrix/formats/fibex.py 0 1 0.0%
Totals Coverage Status
Change from base Build 13389025047: 0.0%
Covered Lines: 4453
Relevant Lines: 8168

💛 - Coveralls

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.

3 participants