search

mrrwa / NmraDcc

NMRA Digital Command Control (DCC) Library
GNU Lesser General Public License v2.1
137 stars 53 forks source link

Does the Case in switch(CV) not require a break? #72

Closed murarduino closed 1 year ago

murarduino commented 1 year ago

I am compiling in EPS-IDF environment and mentioned a warning

`D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp: In function 'uint8_t writeCV(uint16_t, uint8_t)': D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp:616:28: warning: this statement may fall through [-Wimplicit-fallthrough=] DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS);


D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp:618:5: note: here
     case CV_ACCESSORY_DECODER_ADDRESS_LSB: // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS`

I found that there is no "break" at the end of each case of switch (CV). Is this intentional? Is there a problem if we do not deal with this warning?
kiwi64ajs commented 1 year ago

Hmmm… those line numbers are a bit different, but I expect its in this code, right:

uint8_t writeCV (unsigned int CV, uint8_t Value) { switch (CV) { case CV_29_CONFIG: // copy addressmode Bit to Flags Value = Value & ~CV29_RAILCOM_ENABLE; // Bidi (RailCom) Bit must not be enabled, // because you cannot build a Bidi decoder with this lib. DccProcState.cv29Value = Value; DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS); // no break, because myDccAdress must also be reset case CV_ACCESSORY_DECODER_ADDRESS_LSB: // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS case CV_ACCESSORY_DECODER_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB: DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address }

if (notifyCVWrite)
    return notifyCVWrite (CV, Value) ;

if (readEEPROM (CV) != Value)
{
    writeEEPROM (CV, Value) ;

    if (notifyCVChange)
        notifyCVChange (CV, Value) ;

    if (notifyDccCVChange && ! (DccProcState.Flags & FLAGS_SETCV_CALLED))
        notifyDccCVChange (CV, Value);
}

return readEEPROM (CV) ;

}

The answer you’re looking for is also in the code:

// no break, because myDccAdress must also be reset

Feel free to suggest an inline Compiler #Pragma directive, or I guess we could alter the code to duplicate the shared line of code - see bold lines below:

uint8_t writeCV (unsigned int CV, uint8_t Value)

{ switch (CV) { case CV_29_CONFIG: // copy addressmode Bit to Flags Value = Value & ~CV29_RAILCOM_ENABLE; // Bidi (RailCom) Bit must not be enabled, // because you cannot build a Bidi decoder with this lib. DccProcState.cv29Value = Value; DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS);

    DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address
break;

// no break, because myDccAdress must also be reset
case CV_ACCESSORY_DECODER_ADDRESS_LSB:  // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS
case CV_ACCESSORY_DECODER_ADDRESS_MSB:
case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB:
case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB:
    DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address
}

if (notifyCVWrite)
    return notifyCVWrite (CV, Value) ;

if (readEEPROM (CV) != Value)
{
    writeEEPROM (CV, Value) ;

    if (notifyCVChange)
        notifyCVChange (CV, Value) ;

    if (notifyDccCVChange && ! (DccProcState.Flags & FLAGS_SETCV_CALLED))
        notifyDccCVChange (CV, Value);
}

return readEEPROM (CV) ;

}

On 16/06/2023, at 6:55 PM, murarduino @.***> wrote:

I am compiling in EPS-IDF environment and mentioned a warning

D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp: In function 'uint8_t writeCV(uint16_t, uint8_t)': D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp:616:28: warning: this statement may fall through [-Wimplicit-fallthrough=] DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS); ~~~~~^~~~~~~~~~~~~~~ D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp:618:5: note: here case CV_ACCESSORY_DECODER_ADDRESS_LSB: // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS

I found that there is no "break" at the end of each case of switch (CV). Is this intentional? Is there a problem if we do not deal with this warning?

— Reply to this email directly, view it on GitHub https://github.com/mrrwa/NmraDcc/issues/72, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5Y53MAUJNVCGZDUF7LSX3XLP7MJANCNFSM6AAAAAAZI2OI2M. You are receiving this because you are subscribed to this thread.

murarduino commented 1 year ago

Hmmm… those line numbers are a bit different, but I expect its in this code, right: uint8_t writeCV (unsigned int CV, uint8_t Value) { switch (CV) { case CV_29_CONFIG: // copy addressmode Bit to Flags Value = Value & ~CV29_RAILCOM_ENABLE; // Bidi (RailCom) Bit must not be enabled, // because you cannot build a Bidi decoder with this lib. DccProcState.cv29Value = Value; DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS); // no break, because myDccAdress must also be reset case CV_ACCESSORY_DECODER_ADDRESS_LSB: // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS case CV_ACCESSORY_DECODER_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB: DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address } if (notifyCVWrite) return notifyCVWrite (CV, Value) ; if (readEEPROM (CV) != Value) { writeEEPROM (CV, Value) ; if (notifyCVChange) notifyCVChange (CV, Value) ; if (notifyDccCVChange && ! (DccProcState.Flags & FLAGS_SETCV_CALLED)) notifyDccCVChange (CV, Value); } return readEEPROM (CV) ; } The answer you’re looking for is also in the code: // no break, because myDccAdress must also be reset Feel free to suggest an inline Compiler #Pragma directive, or I guess we could alter the code to duplicate the shared line of code - see bold lines below: uint8_t writeCV (unsigned int CV, uint8_t Value) { switch (CV) { case CV_29_CONFIG: // copy addressmode Bit to Flags Value = Value & ~CV29_RAILCOM_ENABLE; // Bidi (RailCom) Bit must not be enabled, // because you cannot build a Bidi decoder with this lib. DccProcState.cv29Value = Value; DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS); DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address break; // no break, because myDccAdress must also be reset case CV_ACCESSORY_DECODER_ADDRESS_LSB: // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS case CV_ACCESSORY_DECODER_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB: DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address } if (notifyCVWrite) return notifyCVWrite (CV, Value) ; if (readEEPROM (CV) != Value) { writeEEPROM (CV, Value) ; if (notifyCVChange) notifyCVChange (CV, Value) ; if (notifyDccCVChange && ! (DccProcState.Flags & FLAGS_SETCV_CALLED)) notifyDccCVChange (CV, Value); } return readEEPROM (CV) ; } … On 16/06/2023, at 6:55 PM, murarduino @.***> wrote: I am compiling in EPS-IDF environment and mentioned a warning D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp: In function 'uint8_t writeCV(uint16_t, uint8_t)': D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp:616:28: warning: this statement may fall through [-Wimplicit-fallthrough=] DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS); ~~~~~^~~~~~~~~~~~~~~ D:/Esp-Doc/ESP-decoder/components/NmraDcc/NmraDcc.cpp:618:5: note: here case CV_ACCESSORY_DECODER_ADDRESS_LSB: // Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS I found that there is no "break" at the end of each case of switch (CV). Is this intentional? Is there a problem if we do not deal with this warning? — Reply to this email directly, view it on GitHub <#72>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5Y53MAUJNVCGZDUF7LSX3XLP7MJANCNFSM6AAAAAAZI2OI2M. You are receiving this because you are subscribed to this thread.

Thanks for the guidance, but I have a few questions:

  1. If you need to add the ID of myDccAddress after each case, why not set DccProcState.myDccAddress = -1; Under the switch?

  2. These cases are currently empty, what is the use? case CV_ACCESSORY_DECODER_ADDRESS_LSB:
    case CV_ACCESSORY_DECODER_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB: case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB:

  3. If there is an extra Railcom function, can I rewrite it like this: **case CV_29_CONFIG:

    ifdef CONFIG_IS_RAILCOM

    Value = Value &  CV29_RAILCOM_ENABLE;

    else

    // copy addressmode Bit to Flags
Value = Value &  ~CV29_RAILCOM_ENABLE;   // Bidi (RailCom) Bit must not be enabled,
// because you cannot build a Bidi decoder with this lib.

    endif**

` uint8_t writeCV (uint16_t CV, uint8_t Value) { switch (CV) { case CV_29_CONFIG:

ifdef CONFIG_IS_RAILCOM

    Value = Value &  CV29_RAILCOM_ENABLE;
#else  
    // copy addressmode Bit to Flags
    Value = Value &  ~CV29_RAILCOM_ENABLE;   // Bidi (RailCom) Bit must not be enabled,
    // because you cannot build a Bidi decoder with this lib.
#endif
    DccProcState.cv29Value = Value;
    DccProcState.Flags = (DccProcState.Flags & ~FLAGS_CV29_BITS) | (Value & FLAGS_CV29_BITS);
    break;
// no break, because myDccAdress must also be reset
case CV_ACCESSORY_DECODER_ADDRESS_LSB:  break;// Also same CV for CV_MULTIFUNCTION_PRIMARY_ADDRESS
case CV_ACCESSORY_DECODER_ADDRESS_MSB:  break;
case CV_MULTIFUNCTION_EXTENDED_ADDRESS_MSB:break;
case CV_MULTIFUNCTION_EXTENDED_ADDRESS_LSB:break;
}

DccProcState.myDccAddress = -1; // Assume any CV Write Operation might change the Address

if (notifyCVWrite)
    return notifyCVWrite (CV, Value) ;

if (readEEPROM (CV) != Value)
{
    writeEEPROM (CV, Value) ;

    if (notifyCVChange)
        notifyCVChange (CV, Value) ;

    if (notifyDccCVChange && ! (DccProcState.Flags & FLAGS_SETCV_CALLED))
        notifyDccCVChange (CV, Value);
}

return readEEPROM (CV) ;

} `

kiwi64ajs commented 1 year ago

Your suggested changes needlessly cause the decoder address to have to be recalculated, which isn't zero cost.

As this is an issue only because you're using the ESP-IDF, can you please try the suggestions here: https://stackoverflow.com/questions/45129741/gcc-7-wimplicit-fallthrough-warnings-and-portable-way-to-clear-them

And let me know if any of them work for your environment to disable the warning, as I'd prefer not to change the code just to satisfy using it in a non-Arduino environment.

Regards Alex Shepherd