Resolve "Add Mobile Device Control" - [merged]

cynex0 commented 6 months ago

Merges 17-mobile-control -> main

Closes #17

cynex0 commented 6 months ago

requested review from @alexkhval

cynex0 commented 6 months ago

In GitLab by @schexi on May 7, 2024, 13:02

added 17 commits

1c520c7e...e6b9503e - 16 commits from branch main
9e1cad04 - Resolve merge conflicts #17

Compare with previous version

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on terminal/terminal.ino line 247

8192 is a magic number and should be defined as a macro

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on app/src/main/java/se/gu/wiomote/activities/remote/Remote.java line 42

Is this a temporary config implementation?

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on app/src/main/java/se/gu/wiomote/activities/remote/Remote.java line 64

Magic numbers should be fixed. Also the code seems repetitive, is there a way to make it more robust?

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on app/src/main/java/se/gu/wiomote/activities/remote/RemoteRecyclerAdapter.java line 70

Magic number

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on app/src/main/java/se/gu/wiomote/activities/Setup.java line 129

Nice code reusage

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on app/src/main/java/se/gu/wiomote/configurations/Configuration.java line 58

Don't forget to mention the format in the documentation

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

Commented on app/src/main/java/se/gu/wiomote/network/WioMQTTClient.java line 39

Nice use of constants!

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 13:53

App connection to the WIO terminal tested, occasional bugs occur when the app does not send any MQTT messages on a button press. Additionally a bug on a terminal side occurs when it does not receive the messages and the terminal needs to be restarted. Needs to be fixed on a separate branch.

Overall nice implementation of the mobile device control! :thumbsup:

cynex0 commented 6 months ago

Yes, will be replaced by the device selection feature that is not a part of this issue.

cynex0 commented 6 months ago

In GitLab by @albu-razvan on May 7, 2024, 13:55

Commented on app/src/main/java/se/gu/wiomote/activities/remote/Remote.java line 64

You could make a list with every view, id and published message, but for this context it doesn't make sense since we'll already have a predefined button to command map later on.

cynex0 commented 6 months ago

In GitLab by @albu-razvan on May 7, 2024, 13:55

Commented on app/src/main/java/se/gu/wiomote/activities/remote/RemoteRecyclerAdapter.java line 70

This will be replaced by the size of the custom configuration button list.

cynex0 commented 6 months ago

In GitLab by @albu-razvan on May 7, 2024, 14:19

Commented on terminal/terminal.ino line 247

We'll add a definition in the next commits.

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 14:19

approved this merge request

cynex0 commented 6 months ago

In GitLab by @alexkhval on May 7, 2024, 14:19

mentioned in commit 890867a1e39ed81473efebd2c851a8baa7c860f0

cynex0 / wiomote

Resolve "Add Mobile Device Control" - [merged] #29