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

Improve sensor detector usability #897

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

patrickelectric
Copy link
Member

Ping1D has an unstable ABR procedure, where it fails to communicate with 115200 or 9600
in a not deterministic way.
Such problem breaks the find device logic, where Ping1D fails to answer 115200 and
answers with 9600, this will make the 9600 baud rate valid and invalidate 115200 baud rates.
Doing the connection with such baudrate, will result in a poor performance for the user.
To avoid such problem, we are going to drop 9600 baud rate and ask the user to use manual connection.
The user can also update the firmware to use the automatic baud rate feature.

We are also hiding the serial baud rate and UDP connection port, making the interface cleaner and less confure.

Since the unstable nature of the ABR detection in Ping1D, this patch also allows the sensor to fail when being detected, the sensor can fail with this patch a max number of 3 times, after that the sensor will be considered unavailable (red circle).

@patrickelectric patrickelectric requested review from jaxxzer, tcanabrava and Williangalvani and removed request for jaxxzer February 19, 2020 20:31
@jaxxzer
Copy link
Member

jaxxzer commented Feb 20, 2020

Hi can you explain this one more. Which firmware version has the problem?

Ping1D has an unstable ABR procedure, where it fails to communicate with 115200 or 9600
in a not deterministic way.

@patrickelectric
Copy link
Member Author

patrickelectric commented Feb 20, 2020 via email

@jaxxzer
Copy link
Member

jaxxzer commented Feb 20, 2020

I think the problem is on the qt side, the Ping1D 3.28 firmware is giving the expected response 100% of the time over here: bluerobotics/ping-python#82

image

100percent.logicdata.zip

@patrickelectric
Copy link
Member Author

@jaxxzer have you tested with ping-viewer master branch ?

@jaxxzer
Copy link
Member

jaxxzer commented Feb 20, 2020

Yes, I've tried it right now and realized the issue you describe.
Try this: #898

qml/DeviceManagerViewer.qml Show resolved Hide resolved
src/devicemanager/devicemanager.cpp Show resolved Hide resolved
src/devicemanager/devicemanager.cpp Outdated Show resolved Hide resolved
continue;
}

const auto indexRow = index(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

explicit types when it's ipossible to guess the type by reading the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

indexRow = index(i) appears to be pretty clear that is an index value. We should also not care what type it's, since it's an argument for dataChanged.
Besides that, this happens in others parts of the code.

src/devicemanager/devicemanager.cpp Show resolved Hide resolved
src/devicemanager/devicemanager.cpp Show resolved Hide resolved
Removes baudrate and UDP port from user interface

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Sensor should deal with automatic baud rate procedure or communicate under 115200

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
…rial filter

Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants