-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt to read differents device modes #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this. I was thinking of doing similar extensions. Good job on investigating output of all those device modes.
break; | ||
} | ||
|
||
case Consts.BoostTiltModes.ACCEL: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change makes the API incompatible since previously this mode, which is the default mode for this sensor was reported as "tilt".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what should be done as it breaks but on the other hand, it use information from the lego side which should be "the truth".
There is a lot of changes in this PR and it definitly could not be merge without a new major version.
const mode = this._getModeForDeviceType(this._portLookup(port).type); | ||
this._deactivatePortDevice(this._portLookup(port).value, this._portLookup(port).type, mode, 0x00, () => { | ||
const { type, value, mode } = this._portLookup(port); | ||
this._deactivatePortDevice(value, type, mode || this._getModeForDeviceType(type), 0x00, () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will switch the port mode to the default for type (_getModeForDeviceType
) if the mode
is 0 since 0 is falsy.
Technically it should be mode === null
but this could only happen when you call unsubscribe
before subscribe
, so not sure if that should be handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure to understand how subscriptions work: I don't get why mode is needed to unsubscribe...
@@ -17,6 +18,7 @@ export class Port { | |||
this.id = id; | |||
this.value = value; | |||
this.type = Consts.DeviceType.UNKNOWN; | |||
this.mode = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is mode
also cleared on device reconnect? If not it could lead into one device "inheriting" the mode of the previous device if you don't use autoSubscribe (which will reset it to default for the device on attach).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not because the mode would be overwritten by the next subscribe (either auto or not, you have to recall subscribe when device change).
However, that's a good point regarding consistency so I fixed it in 3bc8404 . It can also be used to know if the devices already have a subscription.
Here is the mode info for WeDo 2.0 tilt sensor (it does not seem to be able to return the raw accelerometer reading like the boost internal sensor):
And WeDo 2.0 distance sensor
|
|
||
case Consts.BoostTiltModes.TILT: { | ||
values.push(data[4]); | ||
event = 'tilt'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this mode the sensor returns a discrete direction of tilt (1, 3, 5, or 7) for each major direction as far as I remember. This is not a very useful mode IMO, but previously the 'tilt' event was reporting acceleration (boost and control+ hubs) or angles (wedo sensor), so this is another change to the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit that I don't know either why this mode exists as it could be calculated from angle...
Maybe to save some power and require angle only when you have a tilt trigger ?
@@ -7,3 +7,6 @@ export function toHex (value: number, length: number = 2) { | |||
export function toBin (value: number, length: number = 8) { | |||
return value.toString(2).padStart(length, "0"); | |||
} | |||
export function toDistance(value: number, partial: number = 1) { | |||
return Math.floor((value + 1 / Math.max(partial, 1)) * 25.4) - 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this changes the result when partial
is 0. In the old code nothing was added to the inch distance, now you add 1. I am not sure what is the source for the previous formula (I am guessing empirical reverse engineering). At least after the change the result is not negative as often.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to make it a nice oneliner but I missed that is add 25.4 to the final result. It makes the result >5.4 in any case.
I did not understood that partial
value so I tried to get more information. According to a comment on eurobricks from @dlech, the format would be:
<COLOR CODE> <DISTANCE> <LED COLOR> <REFLECTED LIGHT>
If this right, the result should differ by ~24.5 for obstacles with significant reflectivity gap at the same distance.
It also explain why that partial
is not part of the distance mode data. So SPEC_1 and PROX mode does not get the same accuracy in this implementation. I guess the formula should be removed for consistency ?
this is outdated by #60 |
This is not ready to be merged, working on #55 took me further than I expected and I ended trying to read every input mode from every device I have.
Now I need help/advice/review to check what I did wrong and test devices I don't have (WeDo2 hub and accessories, Duplo train base)
To handle different data from different mode I added several events:
Using the debug output I summed up mode numbers and names for each hub.
The bad news is: the same device on different hubs does not expose exactly the same modes.
Color and distance
Motors
BASIC_MOTOR
BOOST_TACHO_MOTOR
BOOST_MOVE_HUB_MOTOR
CONTROL_PLUS_LARGE_MOTOR / CONTROL_PLUS_XLARGE_MOTOR
Tilt/accel sensors
Internal
WeDo tilt sensor
???