Bug reading BCD values when hardware not present or simulated

[whaleygeek]

Hello @shaoziyang

Thank you for contributing this package.

Today, @edbye and I saw a program crash using this package. After much digging, we discovered that you use HexToDec to convert BCD numbers from the device to decimal.

For getWeekday, this causes an erroneous value to be returned to the user app, which can crash the user app.

Because inside the simulator, a digitalWritePin(1) means that the next digitalReadPin() will also return a 1, it means that inside the MakeCode simulator the routine returns FF.

This FF is then converted to Decimal like this:

(255>>4) * 10 + (255 % 16)
15*10 + 15
150 + 15
165

This returns day of week as 165, the user app then indexes into a 7 entry Array of String to turn it into the name of the day, element 165 does not exist, so it uses 'undefined', and a later displayString(undefined) causes the simulator to halt with a sad face.

@edbye also replicated this on the real hardware by holding the DIO pin high, and the user code suffers a similar fate.

Can we recommend that once you convert data, you range check it (and limit it perhaps to zero) in case of an out of range value, so that the user program does not have to do this?

The next operation of the user program is on button press to store a known value in the RTC chip registers - but this existing overflow behaviour creates a strange experience for users in the MakeCode simulator.

e.g. how about

getWeekday(): number {
            let d =HexToDec(this.getReg(DS1302_REG_WEEKDAY + 1))
            if (d < 1 || d > 7 ) return 1 
            return d
        }

Thanks!

VERSION TRACE

"name": "DS1302",
    "version": "1.1.0",
    "description": "makecode DS1302 RTC Package for micro:bit",

"installedVersion": "github:makecode-extensions/ds1302#fa167292547220ee2af362e90a2c86a63fc772db"

Chip valid ranges (from DS1302 datasheet)
https://datasheets.maximintegrated.com/en/ds/DS1302.pdf

[whaleygeek]

Also, @edbye just commented that other values read back from other registers will suffer similar fate. The real issue is using HexToDec to convert what is really a BCD number, the overflow semantics are non desireable.

[shaoziyang]

I will fixed it.