Clean up Shearwater string handling

Dirk Hohndel dirk at hohndel.org
Sun Jul 9 12:06:37 PDT 2017


Your code is a lot more beautiful and better structured than mine. It's
also incorrect.

On Sat, Jul 08, 2017 at 08:27:53PM -0700, Linus Torvalds wrote:
> +// The Battery state is a big-endian word:
> +//
> +//  ffff = not paired / no comms for 90 s
> +//  fffe = no comms for 30 s
> +//
> +// Otherwise:
> +//   - top four bits are battery state (0 - normal, 1 - critical, 2 - warning)
> +//   - bottom 12 bits are pressure in 2 psi increments (0..8k psi)
> +//
> +// This returns the state as a bitmask (so you can see all states it had
> +// during the dive). Note that we currently do not report pairing and
> +// communication lapses. Todo?
> +static unsigned int
> +battery_state(const unsigned char *data)
> +{
> +	unsigned int pressure = array_uint16_be(data);
> +	unsigned int state;
> +
> +	if ((pressure & 0xFFF0) == 0xFFF0)
> +		return 0;
> +	state = pressure >> 12;
> +	if (state > 2)
> +		return 0;
> +	return 1u << state;

I don't think this is right. Actually, see below.

> +// Show the battery state
> +//
> +// NOTE! Right now it only shows the most serious bit
> +// but the code is set up so that we could perhaps
> +// indicate that the battery is on the edge (ie it
> +// reported both "normal" _and_ "warning" during the
> +// dive - maybe that would be a "starting to warn")
> +//
> +// We could also report unpaired and comm errors.
> +static void
> +add_battery_info(shearwater_predator_parser_t *parser, const char *desc, unsigned int state)
> +{
> +	if (state >= 1 && state <= 7) {
> +		static const char *states[8] = {
> +			"",		// 000 - No state bits, not used
> +			"normal",	// 001 - only normal
> +			"critical",	// 010 - only critical
> +			"critical",	// 011 - both normal and critical
> +			"warning",	// 100 - only warning
> +			"warning",	// 101 - normal and warning
> +			"critical",	// 110 - warning and critical
> +			"critical",	// 111 - normal, warning and critical

I know that this is wrong. The state is '0' when the battery is normal.
I haven't seen warning or critical, yet, but I bet that they meant the
comment quite literally: the top four bits give you values from 0 to 15, 0
is normal, 1 is critical, 2 is warning, rest is unused. Being able to
combine them like bits would be just weird and nonsensical. What on earth
does it mean to be both normal and critical?

NAK on this.

/D


More information about the subsurface mailing list