Add new 7.0.0 data model config values.

[kareltucek]

Closes #733.

(Untested.)

[mondalaci]

Ideally, we shouldn't use floats in the user configuration to begin with. It's fine to use them internally, but it'd make more sense to use int8_t or int16_t in the user configuration depending on the desired resolution and serialize/deserialize them to/from floats upon writing/reading.

[kareltucek]

I am afraid I don't understand. Could you elaborate a bit more?

E.g.:

• Why not floats?
• So am I supposed to create int/uint-encoded logarithmic scales for multiplicative factors, or are you implying that all values have to be linear (in the sense of having a constant step between any two possible adjecent values)?
• Wouldn't it be a bit confusing to have the same configurations encoded differently in GUI and in macros? (e.g., -127 to 127 uint8 centered around 0 in gui, while in macro, you would be entering a normal float centered around 1 in range 0 <- 1 -> inf).
• So is Agent supposed to have an interpretation that is semantically similar to the firmware's internal float representation?
• Computing precise exponentials on UHK is darn expensive.

[mondalaci]

I mean linear scales.

To give you an example, on the "Mouse key speed" page, one can specify discrete values using the sliders. Setting the initial mouse pointer speed to 1326.234908 allows for unnecessarily fine-grained resolution, and from a usability standpoint, providing a sensible interval and step size per value would make more sense.

I don't know the intricacies of floating-point representation, but I know that some fractional values, such as 0.1 may end up being 0.0999999, which would be wise to avoid by using an integer-based representation. My example may not be valid, but you get the gist of it.

Agent would use per-property intervals and step values to render values.

[kareltucek]

Well, sorry, but converting an inherently logarithmic and axis-wise-symmetric scale (like the axis skew) into a linear one, and furthermore making that choice permanent by encoding it into the serialization format sounds like a pretty insane idea to me.

(If you wonder, the linearity destroys the axis symmetry, making the scale have much finer resolution for one axis than for the other one.)

I don't know the intricacies of floating-point representation, but I know that some fractional values, such as 0.1 may end up being 0.0999999, which would be wise to avoid by using an integer-based representation. My example may not be valid, but you get the gist of it.

As for this, why not let the GUI handle rounding, stepping, etc? By doing what you propose you are coupling the range and resolution to the serialization format, therefore making them hard to change later as well as confusing to work with.

Anyway, I will not block this further. Are you sure you want what you are asking for?

[mondalaci]

Haven't thought of logarithmic settings. On second thought, I agree that we should store them as float. The space-saving would be negligible, conversion would add extra complexity, and Agent can do rounding as suggested.

[kareltucek]

(Added some fixes.)

[mondalaci]

Please remove SerializedModuleProperty_CursorAxisLock because I don't foresee people using it.

Please prefix module-specific properties with module names, such as SerializedModuleProperty_Touchpad_PinchZoomSpeedDivisor. It's quite a mouthful, but I'd rather be explicit.

SerializedModuleProperty_SwapAxes and SerializedModuleProperty_InvertScrollDirectionX only make practical sense for the key cluster, so please modify them accordingly.

Why have SerializedModuleProperty_InvalidStatePlaceholder?

[kareltucek]

Why have SerializedModuleProperty_InvalidStatePlaceholder?

Because experience has taught me that it is good to have one value reserved for an undefined state. I am using 255 for this purpose at multiple places in the project - e.g., for macro slot identification, or for layer id in layer switcher...

Call it a poor man's maybe monad.

[mondalaci]

Please elaborate on the usefulness of this pattern because I can't see it.

[kareltucek]

Have you ever used NULL? It is basically the same pattern, except for non-pointer type.

[mondalaci]

Having NULL at the end of a string or array makes sense to me, but this key-value use case is different, and I can't see its use.

[kareltucek]

I don't mean NULL as a zero character, I mean NULL in the sense of an undefined pointer.

[kareltucek]

But indeed if need be, it can be allocated somewhere in the middle of the enum... Or as 256, as inmemory representation is not restricted to 8 bits...

[mondalaci]

For the life of me, I still can't understand the purpose of this NULLish value. I think it's an unnecessary safeguard, but if you insist on it, an in-memory 256 value is preferred because it doesn't pollute the binary serialization format.

[kareltucek]

Nope, just removed it.

[mondalaci]

If DoubleTapSwitchLayerTimeout and DoubletapConditionTimeout always contain the same value, I'd love to see one of them removed to minimize redundancy.

We've reviewed this PR with @ert78gb, and he noticed that it's littered with DataModelMajorVersion >= 7 conditions and suggested that we should rather have a single condition block around the end of the relevant function(s) to simplify the application logic of the firmware and Agent. I agree with him. Going forward, the following structure would be the simplest to use:

// We start with generic properties that don't depend on data model versions
...

if (DataModelMajorVersion >= 7) {
    ...
}

if (DataModelMajorVersion >= 8) {
    ...
}

if (DataModelMajorVersion >= 9) {
    ...
}

...

If you're fine with it, maybe also the 6 major version equality check could be refactored according to the above structure in the name of long-term consistency.

[kareltucek]

If you're fine with it, maybe also the 6 major version equality check could be refactored according to the above structure in the name of long-term consistency.

I don't see a way to do that while retaining backward compatibility.

If we want to drop backward compatibility, then the check can be just removed.

---

I can easily unify it into two version checks at the expense of mixing order of the properties. That is without voiding the invariant that things are not rewritten during a dry run.

---

As for version 6, it does not respect the dry run property: i.e., rewrites config values even during dry run, which I believe is a bug (at least by definition/specification). Should that be fixed?

[mondalaci]

Yes, please fix the dry run bug.

[ert78gb]

I saw the new ReadFloat method. Does it use little or big edian encoding? Node-js supports both I just have to know which should I use.

thx for the refactoring.

[kareltucek]

Little endian, same as ReadUInt16.

[kareltucek]

Yes, please fix the dry run bug.

(Done)

[ert78gb]

@kareltucek I am so sorry, I know it was my idea, but I would like to revert the https://github.com/UltimateHackingKeyboard/firmware/pull/737/commits/9832195544ebd15f2b05586eb2ef72cdcca58a35 commit if you don't mind. It is too big change on Agent side and end of the day it is not really necessary for the module configuration.

The Agent side problem is Agent supports json and binary format serialisation. The json format contains the layer name so I need to introduce new enum to support the old 255 base layer Id and the new. I am working on it for a while and the code will really ugly and hard to maintain. The firmware side implementation more elegant

[kareltucek]

@ert78gb sure, no problem!

[mondalaci]

@kareltucek I've started to test the module settings of Agent, and the firmware doesn't respect the selected navigation modes. It always behaves as if "cursor" is set regardless of the actual setting. Please look into it.

[kareltucek]

I can't reproduce this, selected navigation modes work fine.

What I did:

• bump userconfig version to 7.0.0 in package.json in both agent and firmware (Not sure if this step is needed. Firmware will read 7.0.0 config fine, given that agent flashes it. Not sure what conditions are used in Agent.)
• regenerate versions.h with ./generate-versions-h.js --withMd5Sums
• rebuilt and flashed firmware
• rebuilt agent
• reset configuration
• set mod, fn and mouse layer navigation modes set to caret on trackball module
• tested

[mondalaci]

Works well, indeed! I don't know what was the issue on my end.