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

a few bugs in BuildHat code implementation #2123

Open
jeansfish opened this issue Aug 25, 2023 · 4 comments
Open

a few bugs in BuildHat code implementation #2123

jeansfish opened this issue Aug 25, 2023 · 4 comments
Assignees
Labels
bug Something isn't working Priority:3 Work that is nice to have

Comments

@jeansfish
Copy link

  1. ColorLightMatrix should be a passive sensor
  2. ButtonSensor should be an active sensor
  3. IsActiveSensor method in Brick.cs only checks ColourAndDistanceSensor, it should include all other active sensors.
@jeansfish jeansfish added the bug Something isn't working label Aug 25, 2023
@ghost ghost added the untriaged label Aug 25, 2023
@raffaeler
Copy link
Contributor

I'm not familiar with the BuildHat binding. Could you please expand a bit the problems you found?
It would be helpful if you also point to the sensors source code that you believe does not work as it should.

And of course, if you are willing to help with a PR fixing the issues, you would be more than welcome :)

@Ellerbach
Copy link
Member

@jeansfish it's been a long time since I implemented this binding and I don't remember everything. Now, firmware may have change and some elements may have change as well from the specifications. Happy to help.

  • Looking at the code of the button and the definition,, it is not a passive sensor, so it seems properly set at a Sensor (not active, see below comment as well)
  • The color matrix seems to be an active (see also below)
  • Any sensor more than WeDoTilt seems to be active. The code checks any that is higher than the ColourAndDistanceSensor See:
    WeDoTiltSensor = 0x22,
    and here:
    public static bool IsActiveSensor(SensorType sensorType) => sensorType >= SensorType.ColourAndDistanceSensor;

The only potential issue I see is for the 2 WeDo sensors before the color one.

@jeansfish
Copy link
Author

@Ellerbach Thanks for your amazing job. Yes, when I digging into it, as the BuildHat protocol defines, the button is not an active sensor and the color matrix is an active one.
for the IsActiveSensor function, it should be >=SensorType.WeDoTiltSensor. I have done some tests and after code changes, both sensors work as expected.
There's another issue is MediumLinearMotor, it has only 2 modes, so active motor codes need to change.
I'm still testing other motors and sensors and try to find the best PID for the medium linear motor.

@Ellerbach
Copy link
Member

for the IsActiveSensor function, it should be >=SensorType.WeDoTiltSensor. I have done some tests and after code changes, both sensors work as expected.

I did not have all the sensors to test but most of them, so indeed, some may not fully work as expected.

We love PR ;-) So once you have everything working, please submit a PR! fork the repo first then create a branch in your repo, make all your changes, push on your repo and then make a PR. And the git magic will happen :-)

@krwq krwq added Priority:3 Work that is nice to have and removed untriaged labels Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority:3 Work that is nice to have
Projects
None yet
Development

No branches or pull requests

4 participants