Small fixes - #8
Conversation
Georges760
left a comment
Georges760
left a comment
There was a problem hiding this comment.
I will not accept the commit Add Chip address to JobVersionResponse
For other, please justify the need
| let small_core_mask = core_small_core_cnt - 1; | ||
| let small_core_bits = small_core_mask.count_ones(); | ||
| let chunk = ((data[6] as u16) << 8) | data[7] as u16; | ||
| let nonce = u32::from_le_bytes(data[2..6].try_into().unwrap()); |
There was a problem hiding this comment.
please revert this, a BM1370 specific parsing cannot be part of this bm13xx-protocol crate.
If you need to parse the chip_addr frmo the nonce value, please use the bm1370 crate functions in your program.
There was a problem hiding this comment.
What if I will move logic to bm1370 and other ASIC types, then here change core_small_core_cnt to asic that we store in the Chain thanks that we will can invoke that on "generic" ASIC type. Finally I would change the pub chip_addr: usize, to pub chip_addr: Option<usize>, for the chips that we do not know how to decode?
There was a problem hiding this comment.
i don't follow you fully, please make a commit proposal, I will see the result.
As a general rule for these crates: bm13xx-* crates shall never have any chip specific code, only bm1397/bm1366/bm1370 should implement the chip specific code (relative to their architecture or functional specificity)
| 1.0 | ||
| }; | ||
| Duration::from_secs_f32(space / (self.asic.hash_freq().raw() as f32)) | ||
| Duration::from_secs_f32(space / (self.asic.hash_freq().raw() as f32) / 1_000.0) |
There was a problem hiding this comment.
why this ? please justify
There was a problem hiding this comment.
It returns rolling duration = 2200 sec for 4x1370.
It is the dumbest fix that fix only a symptom and not cause.
There was a problem hiding this comment.
humm, not very convincing...
And where the 4 chip factor appear in the formula for your case ?
| ) | ||
| .unwrap(); | ||
| if freq > target_freq { | ||
| if freq == target_freq + HertzU64::kHz(6250) { |
There was a problem hiding this comment.
why ? please justify
logically, freq == target_freq + HertzU64::kHz(6250) is a sub case of freq > target_freq so it should be OK as it is.
What's specific when "Miner already runs" ?
There was a problem hiding this comment.
If we use > then we can not set lower frequency we can only increase frequency.
By saying "Miner already runs" I mean that we initialized the chain and we have already submitted x shared to the pool.
The goal is to be able to adjust frequency up and down without re-initializing whole chain.
There was a problem hiding this comment.
OK, I understand the need to lower the hash freq (and fully support this idea too, and the fact that current aglo doesn't permit it. But I don't see how this patch will fix the issue by itself.
I think we need the also add something on line 1079 to not only Add 6.25MHz to the current freq, but be able to Add or Sub this 6.25MHz in case the current freq is above/below the target freq. Also the if freq > target_freq on line 1080 need to be adapted. And finally the exit condition (here on line 1091) need to be fully re-designed accordingly.
| let small_core_mask = core_small_core_cnt - 1; | ||
| let small_core_bits = small_core_mask.count_ones(); | ||
| let chunk = ((data[6] as u16) << 8) | data[7] as u16; | ||
| let nonce = u32::from_le_bytes(data[2..6].try_into().unwrap()); |
There was a problem hiding this comment.
i don't follow you fully, please make a commit proposal, I will see the result.
As a general rule for these crates: bm13xx-* crates shall never have any chip specific code, only bm1397/bm1366/bm1370 should implement the chip specific code (relative to their architecture or functional specificity)
| 1.0 | ||
| }; | ||
| Duration::from_secs_f32(space / (self.asic.hash_freq().raw() as f32)) | ||
| Duration::from_secs_f32(space / (self.asic.hash_freq().raw() as f32) / 1_000.0) |
There was a problem hiding this comment.
humm, not very convincing...
And where the 4 chip factor appear in the formula for your case ?
| ) | ||
| .unwrap(); | ||
| if freq > target_freq { | ||
| if freq == target_freq + HertzU64::kHz(6250) { |
There was a problem hiding this comment.
OK, I understand the need to lower the hash freq (and fully support this idea too, and the fact that current aglo doesn't permit it. But I don't see how this patch will fix the issue by itself.
I think we need the also add something on line 1079 to not only Add 6.25MHz to the current freq, but be able to Add or Sub this 6.25MHz in case the current freq is above/below the target freq. Also the if freq > target_freq on line 1080 need to be adapted. And finally the exit condition (here on line 1091) need to be fully re-designed accordingly.
2 participants
No description provided.