Skip to content
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

Add MoveHub Support with Internal Motor and Tilt Sensor #134

Merged
merged 13 commits into from
Dec 31, 2020

Conversation

KeyDecoder
Copy link
Contributor

@KeyDecoder KeyDecoder commented Dec 30, 2020

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 and others added 12 commits October 5, 2020 08:13
…hardware network family in the default set of hub properties

Added InternalMotor property for MoveHub to provide direct access to motor built into hub
Moved OnHub Attached Virtual IO message to be handled only during connection to allow devices with automatic virtual ports to register their ports but avoid double handling of attaching device to a port
Tidied up example internal MoveHub example
Implement reading data from MoveHub tilt sensor for angle, tilt, orientation, impacts and acceleration
Note sometimes the acceleration can be larger than an sbyte value and need to investigate this
Implement basic configuration for tilt sensor
Add example of using MoveHub tilt sensor
…for sbyte decoding where if it exceeds min/max for an sbyte it clamps to min/max. This can occur if a value is returned larger than the expected min/max raw value which gives a larger than expected percentage. This is seen with MoveHub tilt sensor acceleration where if you manually accelerate it hard enough it returns raw beyond the min/max.
Add handling of simple 2 axis state as simple orientation enum output instead of raw value
@corneliusmunz
Copy link
Contributor

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

@tthiery
Copy link
Member

tthiery commented Dec 30, 2020

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).

Copy link
Member

@tthiery tthiery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @KeyDecoder (@corneliusmunz) for the excellent contribution.

Below some small correction requests to keep naming consistent. The change in the port value decoder we can leave or disable by the percentage override (I prefer the later but also accept the current code because I will revert it anyway later with #126)

examples/SharpBrick.PoweredUp.Examples/Program.cs Outdated Show resolved Hide resolved
src/SharpBrick.PoweredUp/Devices/DeviceFactory.cs Outdated Show resolved Hide resolved
src/SharpBrick.PoweredUp/Enums/SimpleOrientation.cs Outdated Show resolved Hide resolved
src/SharpBrick.PoweredUp/Enums/TiltFactoryOrientation.cs Outdated Show resolved Hide resolved
src/SharpBrick.PoweredUp/Hubs/Hub_Ports.cs Show resolved Hide resolved
src/SharpBrick.PoweredUp/Hubs/MoveHub.cs Outdated Show resolved Hide resolved
src/SharpBrick.PoweredUp/Hubs/MoveHub.cs Outdated Show resolved Hide resolved
@tthiery
Copy link
Member

tthiery commented Dec 30, 2020

Oh, and update the README with the coverage.

@tthiery
Copy link
Member

tthiery commented Dec 30, 2020

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

@corneliusmunz
Copy link
Contributor

@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 😉 ) 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
Copy link
Member

tthiery commented Dec 30, 2020

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.

Copy link
Contributor

@corneliusmunz corneliusmunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

@KeyDecoder
Copy link
Contributor Author

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

Split movehub internal motor and external motor into seperate examples since not everyone has the external motor plugged in
Renamed DeviceType for movehub internal motor and tilt sensor to include MoveHub in naming so it is clearer what they are
Changed MoveHub Tilt Sensor to disable percentage and removed workaround on port value single encoder when percentage was bigger than an sbyte
Removed tilt calibration since message encoding is not implemented and unsure what this actually does at the moment so safer to remove for now
Tidied up naming for move hub orientation enums
Add null check in Hub when handling attach/detach messages so now logs information when getting attach or detach for a port it does not know about instead of generating an exception and blocking connection to a hub
Added seperate left and right motors to MoveHub and updating naming to be clearer what type of motor and what port they are connected to
@tthiery
Copy link
Member

tthiery commented Dec 31, 2020

Looks good to me.

Excellent work!

Copy link
Contributor

@corneliusmunz corneliusmunz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KeyDecoder Thanks for all the changes. Looks perfect for me. I have run the examples and everything worked 👏
@tthiery Ready for a before year ends merge 😃 🎆

@tthiery tthiery merged commit 9e89765 into sharpbrick:master Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MoveHub (88006)
3 participants