M-Labs
/
thermostat
9
0
Fork 5
Code Issues 10 Pull Requests 7 Releases Wiki Activity

Fix failure on commands ending on whitespace #75

Open
esavkin wants to merge 3 commits from esavkin/thermostat:63-fix-pid-error into master
pull from: esavkin/thermostat:63-fix-pid-error
merge into: M-Labs:master
Conversation 3 Commits 3 Files Changed 3 +86 -52
esavkin commented 2022-12-21 15:25:53 +08:00
Owner
Copy Link

Probably worth implementing proper command checker.
Closes #63

Probably worth implementing proper command checker. Closes #63
sb10q commented 2022-12-21 16:12:00 +08:00
Owner
Copy Link

Shouldn't this be fixed on the firmware side instead to make writing clients (or manual use) easier?

Shouldn't this be fixed on the firmware side instead to make writing clients (or manual use) easier?
👍 1
esavkin commented 2022-12-23 15:18:29 +08:00
Author
Owner
Copy Link

on the firmware side instead

I think it's better to keep both soulutions, since it is also covered by tests now:

$ cargo test --target x86_64-unknown-linux-gnu
   Compiling thermostat v0.0.0 (/home/esavkin/thermostat)
    Finished test [unoptimized + debuginfo] target(s) in 1.19s
     Running unittests (target/x86_64-unknown-linux-gnu/debug/deps/thermostat-7f5ef5f34642c0ac)

running 27 tests
test command_parser::test::parse_center_point ... ok
test command_parser::test::parse_pwm_i_set ... ok
test command_parser::test::parse_center_point_vref ... ok
test command_parser::test::parse_ipv4 ... ok
test command_parser::test::parse_quit ... ok
test command_parser::test::parse_report_mode_off ... ok
test command_parser::test::parse_postfilter_off ... ok
test command_parser::test::parse_report ... ok
test command_parser::test::parse_pwm_max_i_pos ... ok
test command_parser::test::parse_report_mode_on ... ok
test command_parser::test::parse_steinhart_hart ... ok
test command_parser::test::parse_show_ipv4 ... ok
test command_parser::test::parse_save ... ok
test command_parser::test::parse_save_channel ... ok
test command_parser::test::parse_load_channel ... ok
test command_parser::test::parse_ipv4_and_gateway ... ok
test command_parser::test::parse_pid ... ok
test command_parser::test::parse_report_mode ... ok
test command_parser::test::parse_postfilter ... ok
test command_parser::test::parse_pwm_pid ... ok
test pid::test::test_controller ... ok
test command_parser::test::parse_steinhart_hart_set ... ok
test command_parser::test::parse_pwm_max_i_neg ... ok
test command_parser::test::parse_load ... ok
test command_parser::test::parse_postfilter_rate ... ok
test command_parser::test::parse_pid_target ... ok
test command_parser::test::parse_pwm_max_v ... ok

test result: ok. 27 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s
> on the firmware side instead I think it's better to keep both soulutions, since it is also covered by tests now: ```sh $ cargo test --target x86_64-unknown-linux-gnu Compiling thermostat v0.0.0 (/home/esavkin/thermostat) Finished test [unoptimized + debuginfo] target(s) in 1.19s Running unittests (target/x86_64-unknown-linux-gnu/debug/deps/thermostat-7f5ef5f34642c0ac) running 27 tests test command_parser::test::parse_center_point ... ok test command_parser::test::parse_pwm_i_set ... ok test command_parser::test::parse_center_point_vref ... ok test command_parser::test::parse_ipv4 ... ok test command_parser::test::parse_quit ... ok test command_parser::test::parse_report_mode_off ... ok test command_parser::test::parse_postfilter_off ... ok test command_parser::test::parse_report ... ok test command_parser::test::parse_pwm_max_i_pos ... ok test command_parser::test::parse_report_mode_on ... ok test command_parser::test::parse_steinhart_hart ... ok test command_parser::test::parse_show_ipv4 ... ok test command_parser::test::parse_save ... ok test command_parser::test::parse_save_channel ... ok test command_parser::test::parse_load_channel ... ok test command_parser::test::parse_ipv4_and_gateway ... ok test command_parser::test::parse_pid ... ok test command_parser::test::parse_report_mode ... ok test command_parser::test::parse_postfilter ... ok test command_parser::test::parse_pwm_pid ... ok test pid::test::test_controller ... ok test command_parser::test::parse_steinhart_hart_set ... ok test command_parser::test::parse_pwm_max_i_neg ... ok test command_parser::test::parse_load ... ok test command_parser::test::parse_postfilter_rate ... ok test command_parser::test::parse_pid_target ... ok test command_parser::test::parse_pwm_max_v ... ok test result: ok. 27 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s ```
esavkin changed title from Fix client fail on commands ending on whitespace to Fix failure on commands ending on whitespace 2022-12-23 15:21:13 +08:00
esavkin requested review from sb10q 2022-12-23 16:19:20 +08:00
sb10q reviewed 2022-12-27 17:27:27 +08:00
src/command_parser.rs Outdated
@ -555,0 +565,4 @@
assert_eq!(command, command_w);
command
}};
}
sb10q commented 2022-12-27 17:27:27 +08:00
Owner
Copy Link

Testing once for a command that ends with withespace is enough.

Testing once for a command that ends with withespace is enough.
esavkin commented 2023-01-03 10:29:41 +08:00
Author
Owner
Copy Link

No, before this change different commands had different reaction on ending whitespace(s)

No, before this change different commands had different reaction on ending whitespace(s)
sb10q reviewed 2022-12-27 17:27:50 +08:00
pytec/pytec/client.py Outdated
@ -24,3 +24,3 @@
def _command(self, *command):
self._socket.sendall((" ".join(command) + "\n").encode('utf-8'))
self._socket.sendall(((" ".join(command)).strip() + "\n").encode('utf-8'))
sb10q commented 2022-12-27 17:27:50 +08:00
Owner
Copy Link

If the firmware accepts whitespace at the end this is unnecessary. Remove.

If the firmware accepts whitespace at the end this is unnecessary. Remove.
👍 1
sb10q reviewed 2022-12-27 17:28:05 +08:00
src/command_parser.rs Outdated
@ -537,0 +535,4 @@
postfilter,
value(Ok(Command::Dfu), tag("dfu")),
))(input)?;
let (input, _) = end(input)?;
sb10q commented 2022-12-27 17:28:05 +08:00
Owner
Copy Link

Does this still work with two whitespace characters at the end?

Does this still work with two whitespace characters at the end?
sb10q commented 2022-12-27 17:28:20 +08:00
Owner
Copy Link

And please add an explanation in code comments.

And please add an explanation in code comments.
esavkin commented 2023-01-03 10:35:26 +08:00
Author
Owner
Copy Link

Yes, as shown in test cases, it accepts multiple whitespaces

Yes, as shown in test cases, it accepts multiple whitespaces
esavkin force-pushed 63-fix-pid-error from 081b461e31 to 7d2be841c2 2023-01-03 10:38:13 +08:00 Compare
sb10q reviewed 2023-01-03 11:01:52 +08:00
src/command_parser.rs Outdated
@ -555,0 +558,4 @@
extern crate format_bytes;
use format_bytes::format_bytes;
macro_rules! check_ending_whitespace {
sb10q commented 2023-01-03 11:01:52 +08:00
Owner
Copy Link

This makes it sound like it only checks something about whitespace, and says nothing about parsing and returning the result.

This makes it sound like it only checks something about whitespace, and says nothing about parsing and returning the result.
esavkin commented 2023-01-03 11:23:20 +08:00
Author
Owner
Copy Link

I think it is better now

I think it is better now
esavkin force-pushed 63-fix-pid-error from 7d2be841c2 to d566cca7e0 2023-01-03 11:22:31 +08:00 Compare
sb10q reviewed 2023-01-03 11:25:16 +08:00
src/command_parser.rs Outdated
@ -555,0 +558,4 @@
extern crate format_bytes;
use format_bytes::format_bytes;
macro_rules! parse_with_trailing_whitespaces {
sb10q commented 2023-01-03 11:25:16 +08:00
Owner
Copy Link

Still misleading. Makes it sound like it all it does is add whitespaces and then parse.

Still misleading. Makes it sound like it all it does is add whitespaces and then parse.
esavkin commented 2023-01-03 11:28:23 +08:00
Author
Owner
Copy Link

parse_with_trailing_whitespaces_and_check_equality?
I think 7 words is too much, and still it is a bit confusing

parse_with_trailing_whitespaces_and_check_equality? I think 7 words is too much, and still it is a bit confusing
sb10q commented 2023-01-03 18:08:09 +08:00
Owner
Copy Link

parse_chkwhitespace or similar

parse_chkwhitespace or similar
👍 1
esavkin force-pushed 63-fix-pid-error from d566cca7e0 to 34a38b5ae9 2023-01-04 16:23:37 +08:00 Compare
sb10q reviewed 2023-02-08 19:28:06 +08:00
src/command_parser.rs Outdated
@ -555,0 +558,4 @@
extern crate format_bytes;
use format_bytes::format_bytes;
macro_rules! parse_chkwhitespace {
sb10q commented 2023-02-08 19:28:06 +08:00
Owner
Copy Link

I remain unconvinced that checking every command and every command plus whitespace is necessary. Just like the code only deals with the trailing whitespace in only one place and not separately for every command.
I suggest adding just one test that parses one or a few commands and then the same with various whitespaces at the end.
Also helps keep each test doing only one thing.

I remain unconvinced that checking every command and every command plus whitespace is necessary. Just like the code only deals with the trailing whitespace in only one place and not separately for every command. I suggest adding just one test that parses one or a few commands and then the same with various whitespaces at the end. Also helps keep each test doing only one thing.
esavkin commented 2023-02-09 11:02:25 +08:00
Author
Owner
Copy Link

Before this PR, there was an indiscriminative difference between each command, so for some of the commands this test would pass, while other commands would be obviously wrong. And leaving this test only for one command would not guarantee other's commands would work. So this is why I would like to check every command - this will also help to prevent further regresions.
Moreover, these tests do not cost much of the resources.

Before this PR, there was an indiscriminative difference between each command, so for some of the commands this test would pass, while other commands would be obviously wrong. And leaving this test only for one command would not guarantee other's commands would work. So this is why I would like to check every command - this will also help to prevent further regresions. Moreover, these tests do not cost much of the resources.
sb10q commented 2023-02-09 18:06:26 +08:00
Owner
Copy Link

so for some of the commands this test would pass, while other commands would be obviously wrong

Why is that?
This is not my impression from a quick read of the parsing code.

> so for some of the commands this test would pass, while other commands would be obviously wrong Why is that? This is not my impression from a quick read of the parsing code.
esavkin commented 2023-02-10 10:31:17 +08:00
Author
Owner
Copy Link

Some commands have trailing-whitespace handling initially, some - don't. So probably it worths removing these in commands that have it

Some commands have trailing-whitespace handling initially, some - don't. So probably it worths removing these in commands that have it
sb10q commented 2023-02-10 13:15:44 +08:00
Owner
Copy Link

Right, then a pass of cleanup sounds appropriate.

Right, then a pass of cleanup sounds appropriate.
esavkin force-pushed 63-fix-pid-error from 34a38b5ae9 to 7015538b43 2023-03-22 17:29:04 +08:00 Compare
esavkin commented 2023-03-22 17:30:17 +08:00
Author
Owner
Copy Link
[esavkin@chiron:~/thermostat]$ cargo test --target x86_64-unknown-linux-gnu
    Finished test [unoptimized + debuginfo] target(s) in 0.02s
     Running unittests (target/x86_64-unknown-linux-gnu/debug/deps/thermostat-7f5ef5f34642c0ac)

running 33 tests
test command_parser::test::parse_center_point ... ok
test command_parser::test::parse_center_point_vref ... ok
test command_parser::test::parse_fan_auto ... ok
test command_parser::test::parse_fan_show ... ok
test command_parser::test::parse_fan_set ... ok
test command_parser::test::parse_fcurve_default ... ok
test command_parser::test::parse_fcurve_set ... ok
test command_parser::test::parse_hwrev ... ok
test command_parser::test::parse_ipv4 ... ok
test command_parser::test::parse_ipv4_and_gateway ... ok
test command_parser::test::parse_load_channel ... ok
test command_parser::test::parse_load ... ok
test command_parser::test::parse_pid ... ok
test command_parser::test::parse_pid_target ... ok
test command_parser::test::parse_postfilter ... ok
test command_parser::test::parse_postfilter_off ... ok
test command_parser::test::parse_postfilter_rate ... ok
test command_parser::test::parse_pwm_max_i_neg ... ok
test command_parser::test::parse_pwm_max_i_pos ... ok
test command_parser::test::parse_pwm_max_v ... ok
test command_parser::test::parse_pwm_pid ... ok
test command_parser::test::parse_pwm_i_set ... ok
test command_parser::test::parse_quit ... ok
test command_parser::test::parse_report_mode ... ok
test command_parser::test::parse_report_mode_off ... ok
test command_parser::test::parse_report ... ok
test command_parser::test::parse_report_mode_on ... ok
test command_parser::test::parse_save ... ok
test command_parser::test::parse_show_ipv4 ... ok
test command_parser::test::parse_steinhart_hart ... ok
test command_parser::test::parse_save_channel ... ok
test command_parser::test::parse_steinhart_hart_set ... ok
test pid::test::test_controller ... ok

test result: ok. 33 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
```bash [esavkin@chiron:~/thermostat]$ cargo test --target x86_64-unknown-linux-gnu Finished test [unoptimized + debuginfo] target(s) in 0.02s Running unittests (target/x86_64-unknown-linux-gnu/debug/deps/thermostat-7f5ef5f34642c0ac) running 33 tests test command_parser::test::parse_center_point ... ok test command_parser::test::parse_center_point_vref ... ok test command_parser::test::parse_fan_auto ... ok test command_parser::test::parse_fan_show ... ok test command_parser::test::parse_fan_set ... ok test command_parser::test::parse_fcurve_default ... ok test command_parser::test::parse_fcurve_set ... ok test command_parser::test::parse_hwrev ... ok test command_parser::test::parse_ipv4 ... ok test command_parser::test::parse_ipv4_and_gateway ... ok test command_parser::test::parse_load_channel ... ok test command_parser::test::parse_load ... ok test command_parser::test::parse_pid ... ok test command_parser::test::parse_pid_target ... ok test command_parser::test::parse_postfilter ... ok test command_parser::test::parse_postfilter_off ... ok test command_parser::test::parse_postfilter_rate ... ok test command_parser::test::parse_pwm_max_i_neg ... ok test command_parser::test::parse_pwm_max_i_pos ... ok test command_parser::test::parse_pwm_max_v ... ok test command_parser::test::parse_pwm_pid ... ok test command_parser::test::parse_pwm_i_set ... ok test command_parser::test::parse_quit ... ok test command_parser::test::parse_report_mode ... ok test command_parser::test::parse_report_mode_off ... ok test command_parser::test::parse_report ... ok test command_parser::test::parse_report_mode_on ... ok test command_parser::test::parse_save ... ok test command_parser::test::parse_show_ipv4 ... ok test command_parser::test::parse_steinhart_hart ... ok test command_parser::test::parse_save_channel ... ok test command_parser::test::parse_steinhart_hart_set ... ok test pid::test::test_controller ... ok test result: ok. 33 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s ```
This pull request has changes conflicting with the target branch.
  • src/command_parser.rs
You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.
git checkout -b esavkin-63-fix-pid-error master
git pull 63-fix-pid-error

Step 2:

Merge the changes and update on Gitea.
git checkout master
git merge --no-ff esavkin-63-fix-pid-error
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
sb10q
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/thermostat#75
No description provided.
Delete Branch "esavkin/thermostat:63-fix-pid-error"

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?