fix openlcb velocity 14 28 ss conversions

[bakerstu]

The conversion math for the 14 and 28 SS "set" methods was inverted for the MPH factor. This pull requests corrects it.

[balazsracz]

Why do we actually need these functions? Could we delete them? It seems they have a very unintuitive return value -- they seem to be completely useless except for generating DCC track packets. However, they are not that very useful in a command station implementation actually; we ourselves did not use this in the openmrn/src/dcc directory at all. We wrote several entire command station implementations (four if I can count) without relying on them. The best proof of how difficult to use these functions are is that we did not manage to write correct unit tests for them (neither whoever wrote them nor whoever reviewed the code during the check-in or any time in the last 13 years(!!)).

[bakerstu]

Why do we actually need these functions? Could we delete them?

That's really up to @atanisoft, as I believe he is using them, and in his application, getting them in "DCC" format might have actually been helpful. I'm open to a different API if it is more useful for @atanisoft.

[atanisoft]

Why do we actually need these functions?

They are integrated in a number of the DccTrain implementations for generating the DCC speed packets. I'd suggest for now we keep them as it's entirely possible others are using them that we are not aware of.

But if there is a better (newer) API that could replace it we could then shift to that instead and mark these deprecated.

[balazsracz]

The dcc::Train implementations do not use these functions.
There is a packet generation library: https://github.com/bakerstu/openmrn/blob/master/src/dcc/Packet.cxx#L125
which takes unsigned speed input but is otherwise stateless.

Then there is the dcc::Train implementation which uses that to render actual packets based on state: https://github.com/bakerstu/openmrn/blob/master/src/dcc/Loco.hxx#L478 and https://github.com/bakerstu/openmrn/blob/master/src/dcc/Loco.cxx#L160

Confusing the stateful and stateless parts in the APIs is what makes the code unnecessarily complicated and difficult to use and test.

[atanisoft]

The dcc::Train implementations do not use these functions.

Ahh, I stand corrected on that assertion. A very quick scan of all of OpenMRN code shows that these methods are not used internally.

Perhaps we should consider marking these as deprecated and to shift API usage to call mph() with caller code clamping to respective ranges?

[atanisoft]

I believe he is using them, and in his application, getting them in "DCC" format might have actually been helpful

I was initially using them as it was a convenient way to extract them for sending over to non-LCC CS side of things. But I've reworked that code to not leverage these functions anywhere now. I've added a utility function that consumes a Velocity and outputs a DCC speed byte and a similar function that extracts a raw DCC speed step (0-X where X is dependent on the speed step mode).

I think it would make sense to mark these functions all as deprecated and schedule removal since they are complex and mixing two different data types into a single class. We could shift a utility for conversion to the dcc tree if there is a specific need for it there possibly.