← Back to team overview

kicad-developers team mailing list archive

Re: Should SCH_COMPONENT::m_convert default to 1?

 

Yes :) we should do this in a few other places too

On Thu, Mar 28, 2019, 07:53 Mark Roszko <mark.roszko@xxxxxxxxx> wrote:

> Wouldn't it also be smarter to make it an enum? "2" and "1" are already
> annoying values as you have to hunt them down.
>
> On Wed, Mar 27, 2019 at 10:59 PM Jon Evans <jon@xxxxxxxxxxxxx> wrote:
>
>> No, reverting that commit causes the problems to reappear, even after
>> 175a2bc0b.
>>
>> In SCH_EDIT_FRAME::ConvertPart(), the logic appears to toggle the
>> parameter
>> between 1 and 2 (value 0 is not used). It appears that the convert value
>> for a
>> component is never supposed to be 0, even though the convert value for
>> component
>> members (shapes and pins) can be 0 to indicate that they are common to
>> all body
>> styles.
>>
>> In SCH_COMPONENT::GetNetListItem(), the following check is made:
>>
>> if( pin->GetConvert() && ( pin->GetConvert() != GetConvert() ) ) continue;
>>
>> This has the effect that a component with m_convert set to zero,
>> containing pins
>> with m_convert set to 1, will appear to have no pins when netlisting. I
>> just
>> checked some of the demo projects (that were not converted from Eagle, to
>> rule
>> that out), and it appears that we do default to setting m_convert to 1
>> when
>> creating new parts.
>>
>> In DIALOG_EDIT_COMPONENT_IN_SCHEMATIC::TransferDataFromWindow():
>>
>> if( m_cbAlternateSymbol->IsEnabled() ) m_cmp->SetConvert(
>>     m_cbAlternateSymbol->GetValue() ? 2 : 1 );
>>
>> I am out of time to trace this any deeper, but from brief examination of
>> the
>> code, it looks like the m_convert parameter of SCH_COMPONENT should
>> default to
>> 1.  It is initialized this way in all places except for the Eagle
>> conversion,
>> which is probably why this bug was triggered.
>>
>>
>> -Jon
>>
>> On Wed, Mar 27, 2019 at 10:36 AM Wayne Stambaugh <stambaughw@xxxxxxxxx>
>> wrote:
>>
>>> Jon,
>>>
>>> Would you please see if reverting commit ab74445df4 and loading the the
>>> schematic with the 64 bit time stamps has been fixed by commit 175a2bc0b
>>> when you get a chance?  If commit 175a2bc0b resolves the issue, then
>>> your commit should be reverted.
>>>
>>> Thanks,
>>>
>>> Wayne
>>>
>>> On 3/27/2019 9:44 AM, Jon Evans wrote:
>>> > In my investigation of the issue in that forum thread, I found that
>>> > eeschema was choking on the convert value being zero.
>>> >
>>> > In that case, I have misunderstood the code and we should probably
>>> > revert ab74445df4
>>> > That commit fixed the loading/netlisting issues in the forum thread but
>>> > perhaps it fixed it the wrong way.
>>> >
>>> > -Jon
>>> >
>>> > On Wed, Mar 27, 2019 at 8:35 AM Wayne Stambaugh <stambaughw@xxxxxxxxx
>>> > <mailto:stambaughw@xxxxxxxxx>> wrote:
>>> >
>>> >     Oops.
>>> >
>>> >     On 3/27/2019 8:30 AM, Wayne Stambaugh wrote:
>>> >     > Jon,
>>> >     >
>>> >     > I don't think setting m_Convert to 1 is the correct solution
>>> >     unless the
>>> >     > part actually has a convert.  According to the Doxygen comment:
>>> >
>>> >     /**
>>> >       * Shape identification for alternate body styles.  Set 0 if the
>>> item
>>> >       * is common to all body styles.  This is commonly referred to as
>>> >       * DeMorgan style and this is typically how it is used in KiCad.
>>> >       */
>>> >
>>> >     Unless someone changed this behavior this should still be valid.
>>> >
>>> >     >
>>> >     >
>>> >     > On 3/23/2019 9:08 PM, Jon Evans wrote:
>>> >     >> For context, I've been looking at the issues in this forum
>>> >     >>
>>> >     thread:
>>> https://forum.kicad.info/t/before-giving-up-on-kicad-one-last-attempt-erc-misery/15933/63
>>> >
>>> >     This post is about the timestamps not the convert value.
>>> >
>>> >     >>
>>> >     >> I have already pushed a band-aid fix to "repair" these
>>> >     situations, but
>>> >     >> it looks like they are being caused by the EAGLE importer (or at
>>> >     least
>>> >     >> that is one path to this situation coming up)
>>> >     >>
>>> >     >> Specifically, it looks like m_convert is set to 0 in
>>> >     >> SCH_COMPONENT::Init(), but this value is not valid (it should
>>> be at
>>> >     >> least 1 for components).
>>> >     >>
>>> >     >> Are there any cases that I am missing, where it makes sense for
>>> a
>>> >     >> component to have m_convert set to 0?  If not, it's an easy fix
>>> >     in that
>>> >     >> Init() to set it to 1.  If so, we can just update
>>> >     the SCH_EAGLE_PLUGIN
>>> >     >> to call SetConvert() wherever it creates new components.
>>> >     >>
>>> >     >> -Jon
>>> >     >>
>>> >     >> _______________________________________________
>>> >     >> Mailing list: https://launchpad.net/~kicad-developers
>>> >     >> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> >     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> >     >> Unsubscribe : https://launchpad.net/~kicad-developers
>>> >     >> More help   : https://help.launchpad.net/ListHelp
>>> >     >>
>>> >
>>> >     _______________________________________________
>>> >     Mailing list: https://launchpad.net/~kicad-developers
>>> >     Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>>> >     <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>>> >     Unsubscribe : https://launchpad.net/~kicad-developers
>>> >     More help   : https://help.launchpad.net/ListHelp
>>> >
>>>
>> _______________________________________________
>> Mailing list: https://launchpad.net/~kicad-developers
>> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~kicad-developers
>> More help   : https://help.launchpad.net/ListHelp
>>
>
>
> --
> Mark
>

Follow ups

References