sharpbrick / powered-up

.NET implementation of the LEGO PoweredUp Protocol
MIT License
98 stars 19 forks source link

Add MoveHub Support with Internal Motor and Tilt Sensor #134

Closed KeyDecoder closed 3 years ago

KeyDecoder commented 3 years ago

Closes #95 Finish adding MoveHub support based on work done by @corneliusmunz (https://github.com/corneliusmunz/powered-up/tree/feature/add_move_hub).

This handles the hub itself, the internal tacho motor and the internal tilt sensor. The external motor provided in the Star Wars set is also supported when connected (as a MediumLinearMotor).

The vision/light sensor is not implemented currently but will be implemented as part of #103 in the future.

This re-enables the handling of virtual port attach notifications but only during the initial connection. This allows devices such as the MoveHub to notify about it's virtual ports that is has built into it but preserves the existing way manually created virtual ports are handled (which ignore the notification from the hub). This seems best to preserve existing handling for now since I do not have a hub capable of doing virtual ports so cannot validate any changes to how that works.

Also added a clamping of sbyte values where the hub tilt sensor was returning values exceeding the expected maximum which then scaled to percentage values beyond sbyte min/max. Now clamps to min/max values (so if smaller than -127 than outputs -127 or larger than 127 then outputs 127). There didn't seem to be a neater way of handling this.

corneliusmunz commented 3 years ago

@KeyDecoder Many thanks! I will try it today with my Boost set and give you feedback

tthiery commented 3 years ago

Just had an first pass over it. Excellent stuff so far. The sbyte safeguard I have to think through. There is a larger effort planned for these upscaling cases.

I will do later today a detailed review on it (what I see so far mostly naming).

tthiery commented 3 years ago

Oh, and update the README with the coverage.

tthiery commented 3 years ago

If no one disagrees I will release this in a short term 3.4.0 release.

corneliusmunz commented 3 years ago

@KeyDecoder I have tested it with my MoveHub (from the Boost set) and it worked like a charm šŸ‘ šŸ‘ šŸ˜„ Thanks @KeyDecoder for taking over my intial changes. I could connect and can execute the examples. The only thing i have errors ist the following:

I have seen the TiltCalibration method and want to use it but i run here into problems. I have added the following line in the ExampleMoveHubTiltSensor.cs

await device.TiltCalibrate(TiltFactoryOrientation.LayingFlat);

A exception was thrown and i add a missing MessageType initialization in the MoveHubTiltSensor.cs

            var response = await _protocol.SendPortOutputCommandAsync(new PortOutputCommandTiltFactoryCalibrationMessage()
            {
                HubId = _hubId,
                PortId = _portId,
                ModeIndex = ModeIndexCalibration,
                StartupInformation = PortOutputCommandStartupInformation.ExecuteImmediately,
                CompletionInformation = PortOutputCommandCompletionInformation.CommandFeedback,
                Orientation = orientation,
                MessageType = MessageType.PortOutputCommand
            });

Additionnaly the length of the message and the encoding was missing in the PortOutputCommandEncoder.cs After that the code was running without any exceptions and i could write a message to the hub. Unfortunately the Hub responded with the following GenericErrorMessage:

      < 05-00-05-81-06
fail: SharpBrick.PoweredUp.Functions.TraceMessages[0]
      Error - InvalidUse from PortOutputCommand

I have had a look in to the specification (https://lego.github.io/lego-ble-wireless-protocol-docs/index.html#output-sub-command-tiltfactorycalibration-orientation-passcode-n-a) and found that there is a second parameter named PassCode and the PassCode should be equal to Calib-Sensor (whatever that means :wink: ) I have tried to add that parameter to the Message with the Content of the DeviceType but the error message was the same. Additionally i have tried to use the WriteDirect Message instead of the WirteDirectModeData Message type but it was also not successfull.

I stopped then. Maybe @KeyDecoder, @tthiery you have some additonal hints or ideas what to use for the PassCode parameter or how to get it up and running.

My suggestion would be to remove the TiltCalibrate method completle out of the code for the first implementation of the MoveHub and proceed with the working stuff. I think that method is not really important and only needed for some edge cases. The support of the calibration could be added in a later release. What do you think @KeyDecoder, @tthiery

I have exported the changes as a git patch file: tiltCalibrationChanges.txt

tthiery commented 3 years ago

I stayed away from all the calibration methods. Can really ruin your devices when you do not know what you do.

Therefore, I follow @corneliusmunz thoughts on this: just remove it if we do not understand it.

KeyDecoder commented 3 years ago

Removed the Tilt Calibrate method and PortOutputCommandTiltFactoryCalibrationMessage message since better to add it later once someone figures out what it actually does.

tthiery commented 3 years ago

Looks good to me.

Excellent work!