JkBms battery warning messages are ignored

[meikjaeckle]

The expressions in the if-statements of JkBms.cpp from line 324 to 336 are always false, the error flags are never considered:

https://github.com/shining-man/bsc_fw/blob/cfd3028054bc75c0d705002fbb75c79ec196896c/src/devices/JkBms.cpp#L324

I would suggest to use some kind of Bitfield class to handle the bit operations. If exception handling is not an issue, std::bitset could be used. Otherwise following class (I now, it is a template, but the usage is quite simple) could be used. It can also be used to set the bits for the bms error status. We just have to add the definitions for the bit index as well (e.g. BMS_ERR_STATUS_IDX_CHG_OTP, ...)

Just let me know, if I shall fix the issue, and what solution you prefer. The shortest fix would be if((u16_lTmpValue & 0x2) != 0) ....

Example:

const BitfieldT<uint16_t> batWarnMsg
if(batWarnMsg.IsBitSet( 1)) u32_lTmpValue |= BMS_ERR_STATUS_CHG_OTP;

The bitfield template class:

template <typename DATA_TYPE, typename ENABLE = void>
class BitfieldT;

/**
 * @brief  Bitfield template specialization for the unsigned integral types
 * @note std::bitset could be used instead of this method, if exceptions are accepted.
 */
template<typename DATA_TYPE>
class BitfieldT<DATA_TYPE, typename std::enable_if<std::is_unsigned<DATA_TYPE>::value>::type>
{
  using BitfieldType = DATA_TYPE;

  public:
  constexpr explicit BitfieldT(BitfieldType value)
    : _value{value}
  {}

  /**
   * @brief This method can be used to check, if a bit in the bitfield is set
   * @param bitNr The position of the bit to be checked (counting from 0)
   * @return true if the requested bit is set, false otherwise
   */
  constexpr bool IsBitSet(std::size_t bitNr) const
  {
    if (IsBitNrInRange(bitNr))
      return (_value & (static_cast<BitfieldType>(0x1) << bitNr)) != 0;
    else
      return false; // bitNr is NOT in range
  }

   /**
   * @brief This method can be used to set a bit in the bitfield
   * @param bitNr The position of the bit to be set (counting from 0)
   * @return asserts, if bitNr is out of range
   */
  constexpr void SetBit(std::size_t bitNr) const
  {
    if (IsBitNrInRange(bitNr))
    {
      const BitfieldType bitValue = static_cast<BitfieldType>(0x1) << bitNr;
      _value |= bitValue;
      }
    else
    {
      assert(false && "bitNr is NOT in range");
    }
  }

      /**
   * @brief This method can be used to set a bit in the bitfield
   * @param bitNr The position of the bit to be cleared (counting from 0)
   * @return asserts, if bitNr is out of range
   */
  constexpr void ClearBit(std::size_t bitNr) const
  {
    if (IsBitNrInRange(bitNr))
    {
      const BitfieldType bitValue = static_cast<BitfieldType>(0x1) << bitNr;
      _value &= ~bitValue;
    }
    else
    {
      assert(false && "bitNr is NOT in range");
    }
  }

  private:
  constexpr static bool IsBitNrInRange(std::size_t bitNr)
  {
    return (bitNr < std::numeric_limits<BitfieldType>::digits);
  }

  private:
  BitfieldType _value;
};

[shining-man]

Actually, I thought I fixed that recently.

You could also do it like this: if((u16_lTmpValue>>1)&0x1) u32_lTmpValue |= BMS_ERR_STATUS_CHG_OTP;

Or with the macro isBitSet(byte,bit), which does it according to your suggestion.

In your opinion, what would be the advantage of using the template? I have avoided templates so far, so that it is easy to read for everyone. But that can change;-)

[meikjaeckle]

isBitSet(byte,bit) can also be used, it makes it more readable and I think less error prone than using the shift operation several times. The advantage is not the template itself, it is the helper class in total. It gives you more type safety, can be used in several places of the source code with the same syntax and can be tested independently. But it only helps, if it is used in other places as well. The template part just helps to provide better type safety.

Used sensibly and well documented, templates can make the code more readable and can help to detect errors earlier. But if you overdo it, you can also achieve the opposite ;-)

A good example for an easy to use template class which provides the same performance like C-style arrays but additionally provides safety checking mechanisms and other useful things like iterators, and so on, is the std::array. The definition is nearly the same for both, but usage is, as I think, simpler and more safe:

using ArrayType = std::array<int,2>;

void PrintArray(const ArrayType& a1)
{
  for (const auto& s : a1)
    std::cout << s << ' ';
}

void PrintArray(const uint8_t* a2, std::size_t length)
{   
  for (int i = 0; i < length; ++i)
    std::cout << static_cast<int>(a2[i]) << ' ';
}

int main()
{
    ArrayType a1 = { 1, 2 };
    PrintArray(a1);

    uint8_t a2[2] = { 1, 2 };
    PrintArray(a2, sizeof(a2)/sizeof(a2[0]));

    return 0;
}

[shining-man]

Ok, I understand what you see as the advantage of the template. If you want to fix the problem, you can do it with a template.

[meikjaeckle]

ok, I will fix it

[shining-man]

Fehler ist im JK-BMS behoben