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

feat(unit/timer_unit) :First step implementation of timer unit #41 #44

Merged
merged 19 commits into from
May 20, 2024

Conversation

BrahmaMantra
Copy link
Contributor

@BrahmaMantra BrahmaMantra commented Apr 26, 2024

目前支持 "OnActiveSec" , "OnUnitInactiveSec" , "OnUnitActiveSec", "Unit" 字段 for [Timer]。此时会在parse的时候启用所有计时器,要给shell.service配置相应的shell.timer计时器才能启动shell,后续再实现通过systemctl的方法启用。目前在linux上测试没有问题,但无法在DragonOs顺利运行,疑似是内核空间的time实现有点小bug,导致时间不准确(甚至经常在1s内调用Instant::now()会差了9000s),等待后续完善

@fslongjin
Copy link
Member

有两个问题,

  1. PR的名字应当按照DragonOS开发文档里面参与开发那个指南里面的相关内容,加上前缀
  2. 应当要把任务的issue链接贴到PR的描述里

@fslongjin
Copy link
Member

提交代码之前要cargo fmt并且解决warning哈

@fslongjin
Copy link
Member

疑似是内核空间的time实现有点小bug,导致时间不准确(甚至经常在1s内调用Instant::now()会差了9000s)

能否写个小的测试程序,单独开一个仓库,然后把这个问题复现了,接着在dragonos那个仓库那里提issue

@BrahmaMantra
Copy link
Contributor Author

收到(pr前检查时加了个注释忘记fmt了0.0)

@BrahmaMantra BrahmaMantra changed the title 初步实现了timer unit,支持[Timer]的OnActiveSec, OnUnitInactiveSec, OnUnitActiveSec, Unit,字段 feat(unit/timer_unit) :First step implementation of timer unit, supporting "OnActiveSec" , "OnUnitInactiveSec" , "OnUnitActiveSec", "Unit" fields for [Timer] Apr 28, 2024
@BrahmaMantra BrahmaMantra changed the title feat(unit/timer_unit) :First step implementation of timer unit, supporting "OnActiveSec" , "OnUnitInactiveSec" , "OnUnitActiveSec", "Unit" fields for [Timer] feat(unit/timer_unit) :First step implementation of timer unit Apr 28, 2024
@BrahmaMantra BrahmaMantra changed the title feat(unit/timer_unit) :First step implementation of timer unit feat(unit/timer_unit) :First step implementation of timer unit #41 Apr 28, 2024
@BrahmaMantra
Copy link
Contributor Author

This resolves #41

@fslongjin fslongjin requested a review from GnoCiYeH April 28, 2024 10:38
Copy link
Member

@GnoCiYeH GnoCiYeH left a comment

Choose a reason for hiding this comment

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

整体看了一遍,有一些问题,需要注意的是,这个程序是主要跑在DragonOS的,需要再DragonOS实际测试,最好有测试用例。

src/main.rs Outdated
@@ -46,12 +48,12 @@ fn main() {
0
}
};

if id != 0 {
if id != 0 && TimerManager::is_timer(&id) {
Copy link
Member

Choose a reason for hiding this comment

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

这个地方是否有些问题,目前的写法是如果不是timer就不会运行了,这个问题如果在DragonOS测试过的话应该能够第一时间发现的。。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

测试过的,我暂时打算一个service配置一个timer,便于测试OnActiveSec的效果

Copy link
Member

Choose a reason for hiding this comment

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

但是如果现在这个PR合并的话,在DragonOS是跑不了的,所以你可能得先适配一下

src/main.rs Outdated
pub struct FileDescriptor(usize);

const DRAGON_REACH_UNIT_DIR: &'static str = "/etc/reach/system/";
//const DRAGON_REACH_UNIT_DIR: &'static str = "/home/fz/testSystemd/";
Copy link
Member

Choose a reason for hiding this comment

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

这种临时的注释需要删除

@@ -39,13 +50,57 @@ impl TimerManager {
.push(Timer::new(duration, Box::new(callback), parent));
}

pub fn push_timer_unit(unit: Arc<Mutex<TimerUnit>>) {
let timemanager = TIMER_TASK_MANAGER.write().unwrap();
let mut unit_ = unit.lock().unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

这个命名为unit_guard更合适

@@ -92,6 +92,7 @@ impl UnitManager {
}

// 通过id获取到path
// ↑感觉是笔误,应该是通过path获取到id
Copy link
Member

Choose a reason for hiding this comment

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

哈哈哈笔误,可以直接改一下<

String::new(),
0,
));
return self.timer_part.set_attr(attr_type.unwrap(), val);
Copy link
Member

Choose a reason for hiding this comment

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

这里不应该unwrap,应该是向上层抛出,不然别人特意捏一个错误信息,程序就直接就崩了。

where
Self: Sized,
{
// 从给定的路径解析并创建计时器单元
Copy link
Member

Choose a reason for hiding this comment

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

这种注释,标注在函数声明上方更为规范~

}

fn restart(&mut self) -> Result<(), RuntimeError> {
// 重启单元
Copy link
Member

Choose a reason for hiding this comment

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

同理

@GnoCiYeH
Copy link
Member

整体代码很ok,注释的风格可以更加规范一点~,然后就是我在DragonOS测试的时候,发现设置的5s但是触发时间很不稳点,这个bug可以看一看是内核还是dragonreach实现的问题

@fslongjin
Copy link
Member

整体代码很ok,注释的风格可以更加规范一点~,然后就是我在DragonOS测试的时候,发现设置的5s但是触发时间很不稳点,这个bug可以看一看是内核还是dragonreach实现的问题

这个之前确定了是内核问题,但是不知道为啥还没PR @BrahmaMantra

@BrahmaMantra
Copy link
Contributor Author

整体代码很ok,注释的风格可以更加规范一点~,然后就是我在DragonOS测试的时候,发现设置的5s但是触发时间很不稳点,这个bug可以看一看是内核还是dragonreach实现的问题

这个之前确定了是内核问题,但是不知道为啥还没PR @BrahmaMantra

我滴锅,现在就去

@GnoCiYeH GnoCiYeH merged commit 2069cc0 into DragonOS-Community:master May 20, 2024
3 checks passed
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.

3 participants