Skip to content
This repository has been archived by the owner on May 27, 2020. It is now read-only.

FPGA - allow device detect override without an open failure #215

Closed
wants to merge 1 commit into from

Conversation

kanoi
Copy link

@kanoi kanoi commented Jun 5, 2012

Original code works by failing to open an invalid devpath
New version doesn't attempt to open the invalid devpath
Also added the documentation (that wasn't there before)
On certain versions of windows the failure reduces the number of USB ports that can be opened by a single cgminer
Anyone using the old undocumented feature will need to change from using
icarus:/dev/ttyUSB0 to ica:/dev/ttyUSB0
bitforce:/dev/ttyUSB0 to bfl:/dev/ttyUSB0
as per the new documentation

@kanoi
Copy link
Author

kanoi commented Jun 5, 2012

I also aligned the open attempt DEBUG and ERR messages

@luke-jr
Copy link

luke-jr commented Jun 5, 2012

  1. Prefix should continue to use the driver name ("bitforce:"), not the ugly bug of a device type.
  2. How do you ensure "COM1:" and similar don't break (this is my main reason I didn't do it myself)?

@kanoi
Copy link
Author

kanoi commented Jun 5, 2012

  1. This is cgminer not someone else's fork - please keep bugs in some other fork out of your comments
  2. Simple and blatantly obvious - it's 3 characters - problem solved

@luke-jr
Copy link

luke-jr commented Jun 5, 2012

  1. I was talking about a bug you introduced into cgminer.
  2. Driver names aren't 3 characters.

@kanoi
Copy link
Author

kanoi commented Jun 5, 2012

Please stop referring to issues with some other fork that don't exist here - and what has "DRIVER NAME" got to do with an easy way to get rid of the hack in cgminer you created and wouldn't document because you even admitted it was a hack

As stated at the top, the current code hack forces an open failure rather than not attempting to open the device specified in the "-S"
This change fixes that.

READ THE CODE - they are 3 characters - windows COMn ports are 4 characters minimum linux even more.
It's not possible to break it with 3 character names as per the code I wrote.

The bug addressed here is that 64bit Windows7 with more than 6 BFL's will NOT handle the 7th one with the 32bit windows binary.
i.e. the most common windows platform and the most common windows cgminer binary used

@luke-jr
Copy link

luke-jr commented Jun 5, 2012

They aren't 3 characters, that is a bug in this pullreq. They are "icarus:" and "bitforce:". The open failure may not be ideal, but it isn't a bug (the code is designed to ignore it gracefully).

I've not heard anything about this Windows bug, and don't see any way your pullreq would attempt to address it. Nor do I think new bugs should be introduced to make Windows happy.

@kanoi
Copy link
Author

kanoi commented Jun 5, 2012

"I've not heard anything about this Windows bug ..."
Ignorance is now a reason to not fix a bug?

https://bitcointalk.org/index.php?topic=80852.0

Though ... you were logged into IRC #cgminer when I discussed this at length with xDGDZEx
(both times)

@luke-jr
Copy link

luke-jr commented Jun 5, 2012

Looks like that problem is completely unrelated to this pull request, much less fixed by it. In fact, it looks like nobody's even figured out what the problem is.

@kanoi
Copy link
Author

kanoi commented Jun 6, 2012

@luke-jr
Copy link

luke-jr commented Jun 6, 2012

That's a cosmetic bug in the Icarus driver. It should just be logging it as debug-level, not error.

Not attempting to probe it at all as you seem to want here is fine, except for the bug you're introducing by changing the prefix from the driver name to garbage. Fix that and I'll ACK this.

P.S. There's no need to be afraid of using string constants in inline code...

@kanoi
Copy link
Author

kanoi commented Jun 6, 2012

That 'garbage' you are referring to happens to exactly match the driver ".name" in cgminer.
Please don't confuse cgminer code with other forks.
So based on that - this is good to go.

@luke-jr
Copy link

luke-jr commented Jun 6, 2012

It won't be good to go until you fix it.

@kanoi
Copy link
Author

kanoi commented Jun 6, 2012

So, ckolivas, can we remove his veto control over BFL code since he wont accept a code change due to a BFGMiner requirement that isn't relevant to cgminer?
This is an Icarus required change due to the problem raised by xDGDZEx in that last link I posted.
The fix includes changing the BFL code so they both work the same way with this issue and also resolve the issue.

@luke-jr
Copy link

luke-jr commented Jun 6, 2012

Stop putting words in my mouth, you liar.

This change fixes a strictly-cosmetic Icarus bug in a roundabout way. I don't mind that, but it is also changing usage in cgminer since 2.3.x to be more inconsistent for no reason whatsoever.

This has zero impact on xDGDZEx's BFL issues, which still have an unknown cause.

@kanoi
Copy link
Author

kanoi commented Jun 6, 2012

The usage has to be changed to fix the hack you implemented in 2.3.x
That hack is the cause of this problem.

You have already stated above "... except for the bug you're introducing by changing the prefix from the driver name to garbage. Fix that and I'll ACK this."

However, my code uses the driver .name in cgminer (it is, however, different to the driver .name in that other miner)

@luke-jr
Copy link

luke-jr commented Jun 6, 2012

Driver name is .dname. Stop lying to people to make them think this fixes a problem, it doesn't.

@kanoi
Copy link
Author

kanoi commented Jun 6, 2012

The whole point of this change is to use a name that works rather than one that ... as you said above ... "How do you ensure "COM1:" and similar don't break (this is my main reason I didn't do it myself)?" ... that doesn't allow it.

Simple solution, as already stated, use a 3 character name that is unique for the devices.
In cgminer that 3 character name that is unique per device is ... ".name", instead of ".dname"
Both are attributes of the device, who gives a damn if I use a different cgminer field to solve the issue.

There is no "CORRECT" field that must be used in cgminer as long as it is unique - in cgminer there are 2 such fields, a long one .dname and a short one .name
Again you are arguing because this field is not unique in some other fork of cgminer, whereas it is unique here.

@luke-jr
Copy link

luke-jr commented Jun 6, 2012

There is a way cgminer has always done it, and no valid reason to change it. The correct way to handle COM1: etc is to not treat ':' special if it is the last character.

@kanoi
Copy link
Author

kanoi commented Jun 7, 2012

The current way is not documented and wasn't documented by you because (as you said) it was a hack
Thus few people even know about it

My way isn't a hack and works fine and includes documentation

@boinggg
Copy link

boinggg commented Jun 12, 2012

I have tried kanoi patched exe and it is working perfectly with 4 BFLs. Without this patch I had to run 4 cgminers. Very inefficient. Please find a way to approve this pull request. Thank you. A++

@boinggg
Copy link

boinggg commented Jun 12, 2012

Not sure how ANYONE is running multiple BFL on windows?? I swear I've seen screenshots.

@ckolivas ckolivas closed this Jun 12, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants