pull request to a new branch #1

hercule-karuha · 2024-05-19T06:08:44Z

No description provided.

bulesim-rlib/Cargo.lock

bulesim-rlib/src/lib.rs

examples/ten_stage_analysis/src/main.rs

probe-blue/RProbe.bsv

rb_link/src/lib.rs

examples/ten_stage_analysis/src/main.rs

bulesim-rlib/src/lib.rs

rb_link/src/test.rs

hercule-karuha · 2024-05-22T04:51:33Z

@myrfy001 all fixed

myrfy001 · 2024-05-22T15:14:07Z

rb_link/src/server.rs

+    }
+
+    /// Read the earliest cycle messages sent by the probes labeled as "fifo" and "fired", and organize them into a PipeLineState.
+    pub fn get_pipeline_state(&mut self) -> PipeLineState {


What's the purpose of the Server module? From the name, we think that it should be a module that handles connections, e.g., accept connection, retry connection. But what does the server do after receive a packet should not be included in a module called Server. And maybe there will be many different processing logics, should we put all thoes new logics into the Server module? This is a bad smell in code. Use some design pattern to refactor this.

myrfy001 · 2024-05-22T15:15:16Z

rb_link/src/server.rs

+        // let probe_infos = self.probe_infos.clone();
+        let b2r_cache = self.b2r_cache.clone();
+        let r2b_cache = self.r2b_cache.clone();
+        thread::spawn(move || {


When will this thread terminated? what will happen if this thread terminated?

myrfy001 · 2024-05-22T15:17:38Z

README.md

+$ cd <your bsv projest path>
+// add the .a file after the link command
+$ <your link command> ../../bluesim-rlib/target/debug/libblue.a


should have a runable command line, maybe the user doesn't known how to link a .a file to their bluespec simulator. After all, linking a library to simulator is not a normal operation. bluespec also doesn't have much doc about it.

myrfy001 · 2024-05-22T15:19:54Z

bluesim-rlib/src/lib.rs

+        panic!("cycles over flow!");
+    }
+    let mut stream = STREAM.get_or_init(|| {
+        UnixStream::connect(String::from("/tmp/b2rr2b")).expect("Failed to connect to socket")


In fact, .expect and .unwrap is almost the same. Both of them will panic the process. Rust use Result to handle errors, so, what's your idea not to use Result to handle errors?

Beacuse i just don't think the errors are recoverable.

myrfy001 · 2024-05-22T15:24:09Z

examples/TenStage/TenStage.bsv

I think all examples should have a build script as a playground. It can be a make file or a rust build.rs. No matter which way, we should let the user run the demos using a single command line like 'make'. The example should show the users how to build bsv, how to build probe library, how to link them together, hou to run the upper app to use those probes.

myrfy001 · 2024-05-22T15:26:31Z

bluesim-rlib/src/lib.rs

+        panic!("cycles over flow!");
+    }
+    let mut stream = STREAM.get_or_init(|| {
+        UnixStream::connect(String::from("/tmp/b2rr2b")).expect("Failed to connect to socket")


to speed up test, it's common to run multi test process at the same time. A hardcoded local unix-stream will not work when there is multi process running different tests.

myrfy001 · 2024-05-22T15:29:17Z

examples/TenStage/TenStage.bsv

+
+(* synthesize *)
+module mkAdderPipeline(Empty);
+    function gen_fire_probes(Integer x) = mkRProbe(fromInteger(x + 10));


what does those magic numbers mean?

don't use magic numbers, even in testcases or examples.

myrfy001 · 2024-05-22T15:30:54Z

examples/TenStage/TenStage.bsv

+
+(* synthesize *)
+module mkAdderPipeline(Empty);
+    function gen_fire_probes(Integer x) = mkRProbe(fromInteger(x + 10));


by the way, is it possible to use a string as the probe id to make it more meaningful?

myrfy001 · 2024-05-25T06:49:57Z

rb_link/src/server/mod.rs

+            b2r_cache: Arc::new(Mutex::new(HashMap::new())),
+            r2b_cache: Arc::new(Mutex::new(HashMap::new())),
+        }
+    }

    /// Start a thread to run the server.
    /// Create a UnixListener at "/tmp/b2rr2b".


This comment is outdated

myrfy001 · 2024-05-25T06:52:10Z

bluesim-rlib/src/lib.rs

@@ -24,7 +25,11 @@ pub unsafe extern "C" fn get(res_ptr: *mut u8, id: u32, _cycles: u32, size: u32)
        panic!("cycles over flow!");
    }
    let mut stream = STREAM.get_or_init(|| {
-        UnixStream::connect(String::from("/tmp/b2rr2b")).expect("Failed to connect to socket")
+        let socket = match env::var("B2R_SOCKET") {


duplicated code should go into a function

myrfy001 · 2024-05-25T06:58:39Z

README.md

@@ -40,6 +42,7 @@ module mkAdderPipeline(Empty);
        probe.put_data(data + 1);
        put_times <= put_times + 1;
        if(put_times == 9) begin
+            probe.shut_down_server();


No parentheses here

myrfy001 · 2024-05-25T07:02:50Z

examples/AdderPipeline/Makefile

two makefile has a lot in common, can you extract some common part to a seperate file and include them? Suppose you will have many makefile in the future, once you need to change something, you won't need to change every file.

myrfy001 · 2024-05-25T07:11:16Z

rb_link/src/server/mod.rs

+
+impl B2RServer {
+    /// make a new server with socket: /tmp/b2rr2b
+    pub fn new() -> Self {


I think this API should be removed

myrfy001 · 2024-05-25T07:15:16Z

probe-blue/RProbe.bsv

+    endmethod
+endmodule
+
+module mkFIFOFProbe#(Bit#(WORD_WIDTH) id, FIFOF#(t) fifo)(Empty);


Since some id is kept for internal use(e.g., control shutdown), you should check if the user supplied id is conflict with those reserved ids.

myrfy001 · 2024-05-25T07:30:39Z

rb_link/src/server/mod.rs

+/// Cache bidirectional data and send data upon receiving requests.
+pub struct B2RServer {
+    socket_path: String,
+    b2r_cache: Arc<Mutex<HashMap<u32, VecDeque<B2RMessage>>>>,


If the simulation run a long time, will this buffer overflow or lead to OOM?

The getter poped from the cache, i think the message shouldn't wait too long to be processed in debug program.
So I think it won't lead to OOM.

myrfy001 · 2024-05-25T08:23:15Z

After looking into your implement detail, I found that it seems this framework is a offline-analyse framework. But it should also work in a online-analyse style, that is, you can inject data to simulator on the fly, and analyse simulator state on the fly. For example, some probe can be used as a trigger (you can think it as a conditional breakpoint in software debugger), like when some probe's value is True, or some probes' value do AND logic operation and the result is True, then block the simulator and print some mesage.

So, to implement this behivour, the framework shoud be event driven, and you can hook some analyse logic by subscribe to the events.

By the way, the offline analyse is also another way to do debug and it can also be kept.

hercule-karuha · 2024-05-31T13:51:09Z

@myrfy001 fixed

myrfy001 · 2024-06-01T05:42:06Z

rb_link/src/config.rs

We must treat naming seriously. For the name config, it means something that can be tuned. But thoes two things here seems not. Maybe this file should be called consts.rs or something else?

myrfy001 · 2024-06-01T05:59:06Z

rb_link/src/server/mod.rs

+    socket_path: String,
+    running: Arc<AtomicBool>,
+    cycle: Arc<AtomicU32>,
+    b2r_cache: Arc<Mutex<HashMap<u32, VecDeque<B2RMessage>>>>,


Why not use a channel to do cross thread communication? What you did here is almost build a channel by hand and make code verbose.

myrfy001 · 2024-06-01T06:03:15Z

rb_link/src/server/getter.rs

+    }
+
+    /// Get all message send by the probe with id.
+    pub fn get_id_all(&mut self, id: u32) -> Vec<B2RMessage> {


again, naming!
get_id_all means get all IDs, but this method returns messages instead of IDs.

myrfy001 · 2024-06-01T06:03:53Z

rb_link/src/server/getter.rs

+use std::sync::{Arc, Mutex};
+
+/// A getter to get message from the bluesim by id
+pub struct IDGetter {


Naming problem again.

myrfy001 · 2024-06-01T06:08:29Z

rb_link/src/server/getter.rs

+
+    /// Get all the messages sent by the earliest cycle.
+    /// If there are no messages available, it will return an empty Vec.
+    pub fn get_cycle_message(&mut self) -> Vec<B2RMessage> {


naming problem. Why the important "earliest" not show in the method name but show in the comment?

myrfy001 · 2024-06-01T06:13:29Z

rb_link/src/server/getter.rs

+        let mut b2r_cache: std::sync::MutexGuard<HashMap<u32, VecDeque<B2RMessage>>> =
+            self.b2r_cache.lock().expect("Fail to lock b2r_cache");
+
+        for fifo_id in &self.fifos {


Maybe this can be rewrite in rust style using min instead of for loop?

myrfy001 · 2024-06-01T06:16:55Z

rb_link/src/server/getter.rs

+                        // the fifo message len must be 2
+                        assert_eq!(first_message.message.len(), 2);
+                        let b2r_message = messages.pop_front().expect("front error");
+                        if b2r_message.message[0] == 0 {


No magic number here. You should define a struct to represent this kind of message, which has two fields, one for empty and one for full.

If this is a really big project, or message type boost in the future, it will be hard to remember the meaning of each byte index.

myrfy001 · 2024-06-01T06:22:25Z

rb_link/src/server/getter.rs

+/// fire_fules: the fired rules
+pub struct PipeLineState {
+    pub cycle: u32,
+    pub full_fifos: Vec<u32>,


maybe you should define a new type called ProbeID as u32?

And also, if it stores IDs, then you should show it. Otherwise, when someone read code somewhere else in the code base, and see a struct called full_fifos, since there is a s, he can known this field is a Vec or a slice like things. But what is in this slice? He may think they are some struct represent a FIFO object, and won't think they are IDs.

myrfy001 · 2024-06-01T06:42:49Z

rb_link/src/publisher.rs

+
+impl B2RPublisher {
+    /// Creates a new B2RPublisher instance with the specified socket path.
+    pub fn new_with(path: &str) -> Self {


How does this module works with other modules? It will take the unix stream. What if I want to use b2r_cache and r2b_cache while also using Subscriber framework?
As a framework, different part should be able to be used together, not splited.

hercule-karuha and others added 27 commits May 9, 2024 17:10

Update README.md

3efccb2

blue probe

8c236b2

Create LICENSE

6abe977

add put_t and get_t

2bea3b6

add ignore

c4d9c79

bluesim probe ilb

261a6fa

rb-link

adfb73a

add examples

17b899e

clippy fix

a127a7f

remove warning

b9f4b30

add ignore

7a362ef

change to server

feccc7a

change RProbe type

79428ef

pass simple test

dd60176

reduce nest

b30ad1f

add get by id and bycle

678315f

remove debug message

097754b

add tenstage

43bbd59

fix tenstage error

5db36e7

add simple doc and test

cc2b1ae

remove log file

1624a37

clippy fix

f87343c

add defaule

5dbcd91

add ci

06c07d6

fmt

3136b8e

change to deafult

632c964

remove parentheses

73a1294

myrfy001 reviewed May 19, 2024

View reviewed changes

hercule-karuha added 2 commits May 20, 2024 12:59

remove cargo lock

ab365e7

bule to blue

efe0538

hercule-karuha added 9 commits May 20, 2024 13:19

blue typo in ci and readme

9840cf0

fmt

f19ea40

use OnceLock

b1eec7b

change to signle stream && remove sleep

c54e01b

add width defination

4da6837

change test to pass

9f15eda

allow clippy in test

153db15

add fifo probe type

37b1e0b

add pipeline analysis

28f49aa

myrfy001 reviewed May 22, 2024

View reviewed changes

hercule-karuha added 7 commits May 24, 2024 12:42

refactor add getter

d390d01

add shut down & new_with

9aef4c1

add new test

b3ee694

ci fix

5837a9a

channge readme

847c947

add makefile

1db7f0c

add example readme

56199e3

myrfy001 reviewed May 25, 2024

View reviewed changes

hercule-karuha added 3 commits May 29, 2024 17:56

add Publisher & Subscriber

55e4b57

test Publisher

0f8954a

change shutdown

2eba386

myrfy001 reviewed Jun 1, 2024

View reviewed changes

pull request to a new branch #1

Are you sure you want to change the base?

pull request to a new branch #1

Conversation

hercule-karuha commented May 19, 2024

hercule-karuha commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myrfy001 commented May 25, 2024

hercule-karuha commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment