Split bandaid.#101
Conversation
|
@rruchte does this look right to you? I hate to add a patch to our code for a library issue that is not really a show stopper, then the issue gets patched in upstream and we end up having to revert the patch. |
|
I'll clarify that this fine to me, and will probably work as long as we keep using Hamlib 4.6.5. But then let's say Hamlib 4.7 comes out and this is fixed, we decide to move to Hamlib 4.7, then what? |
|
@aknrdureegaesr maybe consider squashing these two commits into one? That way, if we need to revert this in the future when/if Hamlib is patched, we can do a git revert on one commit that would be affected. |
|
I don't see any real problems with this, the more explicit and well-commented frequency logic is a welcome change. I've primarily been looking at the nuts and bolts of the frequency changes (setting the transceiver state twice in rapid succession is probably not good and could potentially cause problems, etc.), the actual logic around how to determine the frequencies is above my pay grade. This kind of hack does offend my delicate sensibilities to an extent, but if it's the only way to work around the problem, c'est la vie. I'm not well-versed enough in hamlib to know whether or not this makes sense. |
|
It makes sense to me because it addresses an issue in Hamlib. It is one of those situations where I think it would be better to host submodules for these libraries (which I already do for Mac): and patch the library instead of the base program source. But MacOS is the only platform set up to use those submodules and it does not pull from upstream unless I decide to update the submodule. This seems to be the second best solution. So @aknrdureegaesr if you are satisfied this works as expected, please just squash the two commits into one and merge it. I think we do want to make it easily revert-able if Hamlib upstream patches their library and fixes this. |
|
I specifically do not wish to merge these two commits into one. The first commit I want to survive long-term. It contains minor general improvements in the audio frequency logic area. "Scout rule": Leave each place a bit better than you found it. - applied to the code, once I had a look at it. The second commit is a quick band-aid. It works, but I'd be happy to see it removed if either
If you want me to, can separate these two commits into distinct PRs. |
Turning around the sign of m_XIT to improve comprehensibility.
ef0d4eb to
1910da1
Compare
|
@aknrdureegaesr not necessary to do two PR's. If happy with it, just merge it. The Hamlib one is plainly revert-able IF Hamlib ever fixes the issue and even then it likely won't hurt anything. |
|
I'll add that I don't have high hopes that Hamlib will fix this any time soon. Since Mike passed away there these sort of things just don't get looked at on a timely basis and they will probably get into 4.7 before it is ever addressed. Since we don't plan on switching away from 4.6.5 any time soon, this is the best that we have to fix it. |
The underlying bug below #94 and probably #16 is a hamlib bug.
This works around that hamlib bug. So it fixes #94.
Also some documentation changes I did on the way to finding out what the problem was.
I also changed the sign of
m_XITinmainwindow.cppso that it is positive in the common case of an audio frequency between 500 and 1500 Hz, instead of, as it used to be, negative. This should make the codesomewhat more comprehensible.