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.

Sign up for GitHub

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

Jump to bottom

[app] setting page #12

Merged
merged 1 commit into from
May 21, 2023
Merged

[app] setting page #12

merged 1 commit into from
May 21, 2023

Conversation

morris13579
Copy link
Contributor

增加設定頁面

  1. 屏幕亮度設定
  2. 屏幕熄滅時間設定
  3. 加入Wifi設定網頁支援OTA以及設定WIFI、MQTT

修正部分排版有Tab與空格問題。

SmallPond
SmallPond reviewed May 20, 2023
View reviewed changes
1.Firmware/src/app/Pages/HASS/HassView.cpp Outdated
@@ -5,8 +5,8 @@
using namespace Page;
#define ITEM_PAD 60
/*
* 默认视图显示： 圆点，原点所在位置 label_value
* 此函数根据需要增加或 hidden 对象
* 默认视图显示： 圆点，原点所在位置 label_value
Copy link
Owner

Choose a reason for hiding this comment

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

这种注释是这样写的，中间行空格开头，不需要把空格删除。

1.Firmware/src/app/Pages/HASS/HassView.cpp Outdated
// lv_obj_remove_style_all(cont);
// lv_obj_set_size(cont, 42, 62);
lv_obj_clear_flag(cont, LV_OBJ_FLAG_SCROLLABLE);
lv_obj_t *cont = lv_obj_create(par);
Copy link
Owner

Choose a reason for hiding this comment

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

这里是把 tab size 换成了 8 个空格吗？ 我们期望是保持4个空格。 另外，这边还是期望专注在当前 PR 的主题上。

只对 “add setting page”这个主题进行必要的修改。代码格式你在新增的代码中保持一致即可，不要修改已有代码，不然整个 PR 就会显得很乱。

也就是说，整个 PR 只应该出现你实现 setting page 相关的修改。

另外，现在你好像把所有修改都放在同一个 commit 中了，导致这个commit 的修改特别大。理想中应有实现功能时代码模块上的划分。比如：

  • 在 memu 中新增 setting 的入口
  • NVS 功能的增加
  • 增加 setting page 页面
  • WiFi server 网页的增加
  • 等等

这样代码会比较清晰

Copy link
Contributor Author

Choose a reason for hiding this comment

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

因為這個檔案有些地方是空格有些是tab，我看使用tab比較多所以改成tab，所以現在這樣才是有對齊的
原本的確是有分開commit的，但是因為改到另一分支最後就在一起了

1.Firmware/src/app/Pages/Playground/Playground.cpp
@@ -175,8 +177,8 @@ void Playground::onEvent(lv_event_t* event)
switch (app) {
case PLAYGROUND_MODE_NO_EFFECTS:
case PLAYGROUND_MODE_FINE_DETENTS:
case PLAYGROUND_MODE_BOUND:
case PLAYGROUND_MODE_ON_OFF:
case PLAYGROUND_MODE_BOUND:
Copy link
Owner

Choose a reason for hiding this comment

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

这里确实是我的问题，混用了 tab 和空格缩进。 vscode 没正常替换 tab 为空格。应该是当时在另一台机器上开发的时候导致的。

1.Firmware/src/hal/hal.h Outdated
@@ -31,7 +31,7 @@ namespace HAL
void motor_init(void);
// void TaskMotorUpdate(void *pvParameters);
int get_motor_position(void);
void update_motor_mode(int mode);
void update_motor_mode(int mode,int init_position);
Copy link
Owner

Choose a reason for hiding this comment

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

两个参数之间加一个空格，void update_motor_mode(int mode, int init_position); int mode,[空格]int init_position

1.Firmware/src/hal/lcd.cpp Outdated
account->Subscribe("MotorStatus");
xTaskCreatePinnedToCore(
TaskLcdBrightnessUpdate,
"MotorThread",
Copy link
Owner

Choose a reason for hiding this comment

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

这里的实现就非常nice了，从 lvgl 中解耦了，比较清晰明了。

这里注意换个名字。

1.Firmware/src/hal/nvs.cpp Outdated
uint8_t value = prefs.getUChar(INIT_KEY , 0);
if(value != INIT_VALUE){
prefs.putUChar(FFAT_KEY, 0);
set_lcd_bk_brightness(50); //default lcd bk
Copy link
Owner

Choose a reason for hiding this comment

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

这里的三个参数应该都可以在 config 文件中配置的呀。不能光配 MQTT，至少 WiFi 应该也要有的。所以最好这种都用宏来配置默认参数。WiFi 等配置 src\secrets.h 在这个文件中。背光等时间可以配置在src\config.h这个文件中

1.Firmware/src/port/display.cpp Outdated
//pinMode(TFT_BLK, OUTPUT);
//digitalWrite(TFT_BLK, HIGH);
HAL::lcd_brightness_init();
HAL::set_lcd_brightness( get_lcd_bk_brightness() );
Copy link
Owner

Choose a reason for hiding this comment

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

函数调用建议不用在内部打空格哈，当然这是我个人的习惯，如果可以，就统一一下。HAL::set_lcd_brightness(get_lcd_bk_brightness()); 这个还有其他几处也是的。

1.Firmware/src/main.cpp
Serial.println("button short clicked");
} else if (event == ButtonEvent::EVENT_CLICKED) {
Serial.println("button clicked");
}
}

void setup() {
Copy link
Owner

Choose a reason for hiding this comment

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

这里本来就是 4 空格的，不用改这里的缩进哈。

1.Firmware/src/main.cpp Outdated
}

void loop() {
unsigned long currentMillis = millis();
unsigned long currentMillis = millis();
Copy link
Owner

Choose a reason for hiding this comment

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

这个文件中的缩进都是正常的哈

@morris13579
Copy link
Contributor Author

已經修改完成

@SmallPond
Copy link
Owner

很棒，你把这些commit都rebase squash成一个吧。

新加的功能你都测试可用了对吧。

@morris13579
Copy link
Contributor Author

我自己使用是沒有問題

SmallPond
SmallPond reviewed May 21, 2023
View reviewed changes
Copy link
Owner

@SmallPond SmallPond left a comment

Choose a reason for hiding this comment

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

还有一个commit 没有 squash 呢，另外还有一个问题如以下review，都squash 一下吧。

我今晚回去在我的旋钮上试用一下应该就能合入了。

恭喜你！ & 谢谢你~

1.Firmware/src/app/Pages/HASS/HassView.cpp Outdated
lv_obj_add_style(cont, &style.edit, LV_STATE_EDITED);

item->cont = cont;
lv_obj_add_style(cont, &style.edit, LV_STATE_EDITED);
Copy link
Owner

Choose a reason for hiding this comment

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

这个文件中的格式还没恢复呢？ 如上所说：

这里是把 tab size 换成了 8 个空格吗？ 我们期望是保持4个空格。 另外，这边还是期望专注在当前 PR 的主题上。

只对 “add setting page”这个主题进行必要的修改。代码格式你在新增的代码中保持一致即可，不要修改已有代码，不然整个 PR 就会显得很乱

SmallPond

This comment was marked as duplicate.

Sign in to view

@morris13579 morris13579 force-pushed the feature-setting branch 2 times, most recently from abe508d to 06e3f95 Compare May 21, 2023 03:37
SmallPond
SmallPond reviewed May 21, 2023
View reviewed changes
Copy link
Owner

@SmallPond SmallPond left a comment

Choose a reason for hiding this comment

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

回家玩了一下，非常棒！还有以下两个细节点，你再看下。

1.Firmware/src/app/Pages/Setting/SettingView.cpp Outdated
SettingView::UpdateBackgroundView(info);
lv_label_set_text_fmt(
ui.label_value,
"%dmins%",
Copy link
Owner

Choose a reason for hiding this comment

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

这里 %dmins% 后面百分号的作用是什么呢？是不是有点问题？ 还有背光配置的 %d% 。而且在10的时候显示 10min， 11 显示 11mins，12 又显示 12min 了。 这里是否直接不显示 min 好了？ 上面很大几个字体显得很突兀。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

這應該是打錯了，我也沒發現。

1.Firmware/src/app/Pages/Setting/SettingView.cpp
wifi_ui_init(root);
m_ui->cont_row = cont_row;

setting_item_create(
Copy link
Owner

Choose a reason for hiding this comment

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

这里的 item 图标 icon 有些小了，看起来有些模糊。 建议这边和我统一的 HASS 的一样， 42*42的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

的確好像改一下比較好

@morris13579
[app] setting page
5fdd344 
fix HassView note format

add a space between function two parameters

replace tab with space in main

update lcd bk thread name

remove spaces between function variables

use define to set config default value

format hassview

fix setting view display text

update setting icon size
@SmallPond SmallPond merged commit ed8af2a into SmallPond:master May 21, 2023
@morris13579 morris13579 deleted the feature-setting branch May 23, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SmallPond SmallPond SmallPond left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@morris13579 @SmallPond