h3n3/kiauh
1
0
Fork 0
Code Issues 46 Pull Requests 10 Packages Projects Releases Wiki Activity

feat: add Obico for Klipper to KIAUH #227

Merged
kennethjiang merged 36 commits from moonraker_obico into master 2022-08-15 19:44:04 +02:00
Conversation 10 Commits 36 Files Changed 11 +607 -54
kennethjiang commented 2022-07-26 00:20:16 +02:00 (Migrated from github.com)
Copy Link

Hi @th33xitus, I think this PR is ready for review now. Sorry for the delay.

Here are a few areas that I will need your feedback on:

  1. I have been naming the systemd service after the pattern of moonraker-obico[-xxx].service, which caused some problem with existing code. Since moonraker-obico has gone live for a while, I don't want to break the compatibility with the users already using it, I had to make changes here and here. The 2nd one is quite ugly. Let me know if you have a better idea.
  2. moonraker-obico can be in an installed but *not linked" status. I made the change here and here. Again this is a bit of inverted dependency because now main_menu.sh and install_menu.sh depend on functions in obico.sh. Will love to hear your feedback here too.

Screenshots for single-instance installation:

Screen Shot 2022-08-01 at 3 17 22 PM
Screen Shot 2022-08-01 at 3 17 46 PM
Screen Shot 2022-08-01 at 3 17 32 PM
Screen Shot 2022-08-01 at 3 18 03 PM

Screenshots for multi-instance installation:

Screen Shot 2022-08-01 at 6 21 59 PM
Screen Shot 2022-08-01 at 6 24 55 PM

Hi @th33xitus, I think this PR is ready for review now. Sorry for the delay. Here are a few areas that I will need your feedback on: 1. I have been naming the systemd service after the pattern of `moonraker-obico[-xxx].service`, which caused some problem with existing code. Since moonraker-obico has gone live for a while, I don't want to break the compatibility with the users already using it, I had to make changes [here](https://github.com/th33xitus/kiauh/pull/227/files#diff-51c13f857871ab2262217665d19263f641f7765a3047b917380c56e6271a0c0bL682) and [here](https://github.com/th33xitus/kiauh/pull/227/files#diff-ba13e70564a03881545708297775e45ad530fe9daf77c3a4e7577f88c2d75fc8L20). The 2nd one is quite ugly. Let me know if you have a better idea. 2. `moonraker-obico` can be in an *installed* but *not linked" status. I made the change [here](https://github.com/th33xitus/kiauh/pull/227/files) and [here](https://github.com/th33xitus/kiauh/pull/227/files#diff-4a291dc3dd20a7014d941b94a8610be1e17eda608a475fe91514b80cc79a8475R15). Again this is a bit of inverted dependency because now `main_menu.sh` and `install_menu.sh` depend on functions in `obico.sh`. Will love to hear your feedback here too. Screenshots for single-instance installation: ![Screen Shot 2022-08-01 at 3 17 22 PM](https://user-images.githubusercontent.com/779786/182271332-bca22dd0-75fb-4bff-abfb-fe7412abb39a.png) ![Screen Shot 2022-08-01 at 3 17 46 PM](https://user-images.githubusercontent.com/779786/182271327-fc3dc0fe-300c-4005-a7ec-9340b8c5a866.png) ![Screen Shot 2022-08-01 at 3 17 32 PM](https://user-images.githubusercontent.com/779786/182271329-34517d9a-7171-4cd7-b6aa-1e2f81ab5c9d.png) ![Screen Shot 2022-08-01 at 3 18 03 PM](https://user-images.githubusercontent.com/779786/182271326-0ac1750f-1958-4215-9995-baf283fdbc87.png) Screenshots for multi-instance installation: ![Screen Shot 2022-08-01 at 6 21 59 PM](https://user-images.githubusercontent.com/779786/182271875-5e4de181-90dc-41bc-8e98-4773f51a4fd2.png) ![Screen Shot 2022-08-01 at 6 24 55 PM](https://user-images.githubusercontent.com/779786/182271873-fdf7caad-2cd2-46b5-b454-35da151c6092.png)
dw-0 commented 2022-07-27 20:05:41 +02:00 (Migrated from github.com)
Copy Link

Hi @kennethjiang, on first glance it looks like its going into the right direction. I saw one or two oddities, but as you stated that this is a rough draft, i'll wait until you give the go for me to start the formal review.

For example:

Hi @kennethjiang, on first glance it looks like its going into the right direction. I saw one or two oddities, but as you stated that this is a rough draft, i'll wait until you give the go for me to start the formal review. For example: * i don't think that we need a python 3 check, as obico requires moonraker and moonraker requires python 3. * also im not a fan of using the `.*` wildcard in the regex here: https://github.com/th33xitus/kiauh/pull/227/files#diff-08e65de842c3dbbcc98be9e5467e5447241080fbb0bcb661f162d58df16e6a60R20 * this is probably just kind of a copy & paste "placeholder"? https://github.com/th33xitus/kiauh/pull/227/files#diff-08e65de842c3dbbcc98be9e5467e5447241080fbb0bcb661f162d58df16e6a60R242
dw-0 (Migrated from github.com) reviewed 2022-08-13 11:53:01 +02:00
scripts/obico.sh
dw-0 (Migrated from github.com) commented 2022-08-13 11:51:56 +02:00
Copy Link
local log="${KLIPPER_LOGS}"

This variable seems to be unused?

```ssh local log="${KLIPPER_LOGS}" ``` This variable seems to be unused?
scripts/obico.sh
@@ -0,0 +92,4 @@
### Step 1: Ask for the number of moonraker-obico instances to install
if (( moonraker_count == 1 )); then
ok_msg "Moonraker installation found!\n"
new_moonraker_obico_count=1
dw-0 (Migrated from github.com) commented 2022-08-13 11:51:06 +02:00
Copy Link
new_moonraker_obico_count=1

I'm unsure about the usage of this variable. It seems to be not used anywhere? Is it used somewhere else as it is not declared as a local variable?

```ssh new_moonraker_obico_count=1 ``` I'm unsure about the usage of this variable. It seems to be not used anywhere? Is it used somewhere else as it is not declared as a local variable?
kennethjiang (Migrated from github.com) reviewed 2022-08-13 16:36:12 +02:00
scripts/obico.sh
@@ -0,0 +92,4 @@
### Step 1: Ask for the number of moonraker-obico instances to install
if (( moonraker_count == 1 )); then
ok_msg "Moonraker installation found!\n"
new_moonraker_obico_count=1
kennethjiang (Migrated from github.com) commented 2022-08-13 16:36:12 +02:00
Copy Link

Good catch. It's a bug! Do you want me to push this change?

diff --git a/scripts/obico.sh b/scripts/obico.sh
index 791fcdb..de2bd3f 100644
--- a/scripts/obico.sh
+++ b/scripts/obico.sh
@@ -98,7 +98,6 @@ function moonraker_obico_setup_dialog() {
     ### Step 1: Ask for the number of moonraker-obico instances to install
     if (( moonraker_count == 1 )); then
       ok_msg "Moonraker installation found!\n"
-      new_moonraker_obico_count=1
     elif (( moonraker_count > 1 )); then
       top_border
       printf "|${green}%-55s${white}|\n" " ${moonraker_count} Moonraker instances found!"
@@ -134,7 +133,7 @@ function moonraker_obico_setup_dialog() {
         (( new_moonraker_obico_count > allowed_moonraker_obico_count )) && error_msg "Number of Moonraker-obico instances larger than installed Moonraker instances"
       done && select_msg "${new_moonraker_obico_count}"
     else
-      log_error "Internal error. new_moonraker_obico_count of '${new_moonraker_obico_count}' not equal or grather than one!"
+      log_error "Internal error. moonraker_count of '${moonraker_count}' not equal or grather than one!"
       return 1
     fi
Good catch. It's a bug! Do you want me to push this change? ``` diff --git a/scripts/obico.sh b/scripts/obico.sh index 791fcdb..de2bd3f 100644 --- a/scripts/obico.sh +++ b/scripts/obico.sh @@ -98,7 +98,6 @@ function moonraker_obico_setup_dialog() { ### Step 1: Ask for the number of moonraker-obico instances to install if (( moonraker_count == 1 )); then ok_msg "Moonraker installation found!\n" - new_moonraker_obico_count=1 elif (( moonraker_count > 1 )); then top_border printf "|${green}%-55s${white}|\n" " ${moonraker_count} Moonraker instances found!" @@ -134,7 +133,7 @@ function moonraker_obico_setup_dialog() { (( new_moonraker_obico_count > allowed_moonraker_obico_count )) && error_msg "Number of Moonraker-obico instances larger than installed Moonraker instances" done && select_msg "${new_moonraker_obico_count}" else - log_error "Internal error. new_moonraker_obico_count of '${new_moonraker_obico_count}' not equal or grather than one!" + log_error "Internal error. moonraker_count of '${moonraker_count}' not equal or grather than one!" return 1 fi ```
kennethjiang (Migrated from github.com) reviewed 2022-08-13 16:37:53 +02:00
scripts/obico.sh
kennethjiang (Migrated from github.com) commented 2022-08-13 16:37:53 +02:00
Copy Link

Another good catch. I guess I planed to use it but ended up using "${KLIPPER_LOGS}" directly. Feel free to remove it.

Another good catch. I guess I planed to use it but ended up using `"${KLIPPER_LOGS}"` directly. Feel free to remove it.
kennethjiang commented 2022-08-14 06:02:52 +02:00 (Migrated from github.com)
Copy Link

@th33xitus I changed this PR based on our discussion and tested all the cases I can think of.

I found 1 bug: on the install_menu screen, if the user selects 1) [klipper], 2) [moonraker], and 9) [Obico for Klipper] without going back to the main screen, function get_multi_instance_names will return an empty string.

I did some debugging and found that .kiauh.ini is not updated until the user goes back to the main screen. However, I struggled to figure out where I did wrong. Will appreciate it if you can provide some guidance here.

Please also go over the code again, particularly commits 7837a78c4b, 7837a78c4b, and 8d284179d8

@th33xitus I changed this PR based on our discussion and tested all the cases I can think of. I found 1 bug: on the install_menu screen, if the user selects 1) [klipper], 2) [moonraker], and 9) [Obico for Klipper] without going back to the main screen, function `get_multi_instance_names` will return an empty string. I did some debugging and found that `.kiauh.ini` is not updated until the user goes back to the main screen. However, I struggled to figure out where I did wrong. Will appreciate it if you can provide some guidance here. Please also go over the code again, particularly commits 7837a78c4b6113e76dc883058118c0a21eef3410, 7837a78c4b6113e76dc883058118c0a21eef3410, and 8d284179d8a21f7ef6720d5c9388a27e7fce58c5
dw-0 commented 2022-08-14 09:06:04 +02:00 (Migrated from github.com)
Copy Link

I found 1 bug: on the install_menu screen, if the user selects 1) [klipper], 2) [moonraker], and 9) [Obico for Klipper] without going back to the main screen, function get_multi_instance_names will return an empty string.

I did some debugging and found that .kiauh.ini is not updated until the user goes back to the main screen. However, I struggled to figure out where I did wrong. Will appreciate it if you can provide some guidance here.

You didn't do any wrong. You actually pointed me to a maybe not so smart solution.
https://github.com/th33xitus/kiauh/blob/master/scripts/ui/main_menu.sh#L85

The init_ini function is called whenever the main menu is loaded.
https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh#L125

And in there the multi instance names are then written to the ini
https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh#L189

Maybe it is smarter to factor fetch_webui_ports and fetch_multi_instance_names out of the init_ini function and place them both into the install_menu function right after install_ui. Both those fetch functions aquire data that is usually only used in installation functions, at least i can't think of any other usecases within the script right now.

> I found 1 bug: on the install_menu screen, if the user selects 1) [klipper], 2) [moonraker], and 9) [Obico for Klipper] without going back to the main screen, function get_multi_instance_names will return an empty string. I did some debugging and found that .kiauh.ini is not updated until the user goes back to the main screen. However, I struggled to figure out where I did wrong. Will appreciate it if you can provide some guidance here. You didn't do any wrong. You actually pointed me to a maybe not so smart solution. https://github.com/th33xitus/kiauh/blob/master/scripts/ui/main_menu.sh#L85 The `init_ini` function is called whenever the main menu is loaded. https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh#L125 And in there the multi instance names are then written to the ini https://github.com/th33xitus/kiauh/blob/master/scripts/utilities.sh#L189 Maybe it is smarter to factor `fetch_webui_ports` and `fetch_multi_instance_names` out of the `init_ini` function and place them both into the `install_menu` function right after `install_ui`. Both those fetch functions aquire data that is usually only used in installation functions, at least i can't think of any other usecases within the script right now.
dw-0 commented 2022-08-14 15:38:36 +02:00 (Migrated from github.com)
Copy Link

@kennethjiang so i just pushed a commit addressing the bug you mentioned.

I have a view comments to make on other pieces of the code, i quite don't understand yet.

@kennethjiang so i just pushed a commit addressing the bug you mentioned. I have a view comments to make on other pieces of the code, i quite don't understand yet.
dw-0 (Migrated from github.com) reviewed 2022-08-14 15:44:40 +02:00
scripts/obico.sh
dw-0 (Migrated from github.com) commented 2022-08-14 15:40:07 +02:00
Copy Link

Can this variable be local? Or is it required to be global? I see it only being used inside the moonraker_obico_needs_linking function

Can this variable be local? Or is it required to be global? I see it only being used inside the `moonraker_obico_needs_linking` function
dw-0 (Migrated from github.com) commented 2022-08-14 15:41:04 +02:00
Copy Link

This variable seems to be unused now?

This variable seems to be unused now?
dw-0 (Migrated from github.com) commented 2022-08-14 15:44:14 +02:00
Copy Link

See comment above :)

See comment above :)
dw-0 (Migrated from github.com) commented 2022-08-14 15:44:24 +02:00
Copy Link

See comment above :)

See comment above :)
scripts/obico.sh
@@ -0,0 +198,4 @@
### Step 7: Link to the Obico server if necessary
local not_linked_instances=()
if (( moonraker_count == 1 )); then
if moonraker_obico_needs_linking "$(moonraker_obico_config 0)"; then
dw-0 (Migrated from github.com) commented 2022-08-14 15:43:41 +02:00
Copy Link

The moonraker_obico_config function defined in line 24 does not take in any arguments. So the 0 you pass there (and a few lines below the ${i}) won't get used inside the function. Im not quite sure what the intention was, if the arguments are obsolete or if the moonraker_obico_config function is incomplete?

The `moonraker_obico_config` function defined in line 24 does not take in any arguments. So the `0` you pass there (and a few lines below the `${i}`) won't get used inside the function. Im not quite sure what the intention was, if the arguments are obsolete or if the `moonraker_obico_config` function is incomplete?
scripts/obico.sh
@@ -0,0 +249,4 @@
if (( moonraker_count == 1 )); then
status_msg "Link moonraker-obico to the Obico Server..."
"${MOONRAKER_OBICO_DIR}/scripts/link.sh" -q -c "$(moonraker_obico_config 0)"
dw-0 (Migrated from github.com) commented 2022-08-14 15:44:20 +02:00
Copy Link

See comment above :)

See comment above :)
kennethjiang (Migrated from github.com) reviewed 2022-08-14 16:08:08 +02:00
scripts/obico.sh
@@ -0,0 +198,4 @@
### Step 7: Link to the Obico server if necessary
local not_linked_instances=()
if (( moonraker_count == 1 )); then
if moonraker_obico_needs_linking "$(moonraker_obico_config 0)"; then
kennethjiang (Migrated from github.com) commented 2022-08-14 16:08:07 +02:00
Copy Link

This is a really good catch! The function moonraker_obico_config is supposed to use ${1}, not ${i}. Not sure how my testing passed... Fixing it now.

This is a really good catch! The function `moonraker_obico_config` is supposed to use `${1}`, not `${i}`. Not sure how my testing passed... Fixing it now.
kennethjiang (Migrated from github.com) reviewed 2022-08-14 16:16:54 +02:00
scripts/obico.sh
@@ -0,0 +198,4 @@
### Step 7: Link to the Obico server if necessary
local not_linked_instances=()
if (( moonraker_count == 1 )); then
if moonraker_obico_needs_linking "$(moonraker_obico_config 0)"; then
kennethjiang (Migrated from github.com) commented 2022-08-14 16:16:53 +02:00
Copy Link

Should be fixed in 276c123. moonraker_obico_config should take an argument, which is the index of a specific instance in multi_instance_names

Should be fixed in [276c123](https://github.com/th33xitus/kiauh/pull/227/commits/276c123d3d6d0d3609e0f4b179b545d7d53f40cd). `moonraker_obico_config` should take an argument, which is the index of a specific instance in `multi_instance_names`
Sign in to join this conversation.
Reviewers
No Reviewers
dw-0
kennethjiang
Labels
Clear labels
🐛 bug
Something isn't working
 can't reproduce
➕ community extension
crowsnest
duplicate
This issue or pull request already exists
 ✨ feature request
New feature or request
 fluidd
Something that is fluidd related
 improvement
improvement of existing features/code
 invalid
klipper
Something that is klipper related
 klipperscreen
Something that is klipperscreen related
 mainsail
Something that is mainsail related
 mobileraker
moonraker
Something that is moonraker related
 not kiauh
not planned
octoeverywhere
Something that is OctoEverywhere related
 ❓ question
Further information is requested
 💤 stale
todo
wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
h3n3
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: h3n3/kiauh#227
Delete Branch "moonraker_obico"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?