-
Notifications
You must be signed in to change notification settings - Fork 94
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
Port TCPNJE support from spinhawk to SDL Hyperion #280
Comments
This is coming as a surprise to me. Last month, I was copied on an email which suggested I was in discussions about this happening which was also news to me. My original intention was to provide a common codebase for tcpnje which would be compatible with and usable with all Hercules variants and which would work on the widest possible range of host platforms without changes and if I had been successful, no porting would have been required, it would just work. Unfortunately, I have been unable to keep up with incompatibilities and changes detrimental to portability which have been introduced in SDL Hyperion and therefore having common code which works across all Hercules variants has become an impossibility for me. I have been unable to build SDL Hyperion since sometime around 2018. I am not in a position to provide support for tcpnje in SDL Hyperion. I would like all indications that I might provide support for this arrangement removed from any code and documentation included with SDL Hyperion. |
Peter, I cannot remember exactly when it was nor exactly what it was, but I do specifically remember you mentioning an incompatibility between SDL Hyperion and the other Herculeses (*) out there at some point in the recent past (last year or two?), which I also specifically remember having fixed for you! What incompatibilities are you now talking about? If there are incompatibilities in SDL Hyperion that is preventing your tcpnje driver from building or working correctly with SDL Hyperion, you should create a GitHub Issue for it/them so that we (myself and the rest of the SDL Hyperion team) can work on trying to get them fixed for you! Thanks. (*) How the heck do you pluralize "Hercules"? |
I am not aware that there is any. But if you can find any I'd be happy to remove it. Just point it out and it'll be gone. |
Fish, You fixed what feels like dozens of incompatibilities and portability issues for me dating back to well before SDL Hyperion came to exist. The problem for me is these issues continue to arise quicker than I can spot them, report them and ask for them to be fixed. I don't want to be on that treadmill. The biggest issue for me is that support for static linked executables was removed from SDL Hyperion source (in 2018 I think). This made it practically impossible to build SDL Hyperion on any platform which is neither Windows nor one of a limited range of Unix variants. Since then, as far as I can see, it has only been possible to use one of two highly platform specific methods of generating dynamically linked executables, a Windows specific method and a very non-portable Unix specific method coded in ltdl.c / hdl.c. Neither of these methods of building dynamically linked executables can be used in my environment without major surgery which would be all the more difficult due to the almost complete lack of descriptive comments in ltdl.c in particular. (I tried and gave up.) It also makes it so much more difficult to share any common code between SDL Hyperion and any other Hercules variants. After my experience last year asking for a tiny incompatibility to be fixed (which you did fix - thanks) I felt it would be a mammoth task to ask for the restoration of support for statically linked executables [1] and even if this happened, being unable to build SDL Hyperion for so long now, I would not be aware of what new difficulties may have arisen for me since the last time I could build it, which from previous experience could be quite a few. [1] I only made use of the support for statically linked executables present in the actual C source code which I think amounted to a few hundred lines of preprocessor macros and suchlike across all the codebase. (It was probably hard work to remove them and is likely nigh on impossible to replace them all correctly now.) I never used any of the supplied build procedures, makefiles, configure scripts, shell scripts, cmd/bat files, libtools or anything else like that which are all rumoured to not be able to properly generate statically linked executables anyway. |
There are (probably) not any statements involving me in the current SDL Hyperion codebase that I would object to but there will be if the proposal in the title of this thread goes ahead. |
One of the thing I ran into is that there is a mixture of the use of "TCPNJE" as if it where a "typedef" and "struct TCPNJE". Possibly some compilers self correct this, but gcc doesn't. Once I fixed that, there was only a couple of compilation errors that were easy to fix (mostly the missing definition of the variable "filename" which is implicitly used in a convenience macro. So now it compiles, but I haven't tried using it yet. |
What the.... A couple tweaks here and there and.. At least I have the "TCPNJE" device online to RSCS... It's not connecting yet, but that must be something else. But all the module loading and such is resolved (at least under linux). |
Auxiliary question: Would the VM/370 NJE modification support CTC instead of BSC? |
So far I am getting from my TCPNJE capable RSCS subsystem this message when connecting to the TCP/NJE driver:
|
I don't see why this should be an issue. The same is true for SYSBLK, REGS and various other typedefs/structs. Or at least it was true the last time I was able to build this variant.
I expect this is a result of some incompatible code divergence in SDL Hyperion.
No. |
Nowhere is "TCPNJE" typedef'd... But there are plenty of portion of the code that assume it is. And the use is inconsistent... Sometimes it's "struct TCPNJE", sometimes it's just "TCPNJE".. I mean in the code. |
It is in htypes.h |
It's a shame the RSCS NJE mod doesn't support CTC... It'd be closer to the spirit when doing TCP/NJE. |
Not in the current version.. htypes.h does NOT define TCPNJE as a typedef! Anyway, not the real problem. It'd be cool if it were "consistent". The code contains a mixture of "struct TCPNJE" and "TCPNJE". Either all should be one or the other, but not a mix. Not sure why it's not simply a typedef in tcpnje.h! (and shouldn't be used anywhere else than in tcpnje.c) This is an opaque structure to any other part of hercules isn't it? |
htypes.h contains stuff that are pertinent to multiple components of hercules or as a whole... "struct TCPNJE" is a control block that is only relevant to the tcp/nje handler... it doesn't (and shouldn't) contain any information that is relevant to any other portions of the engine. |
This was always my preferred option. I have an RSCS mod to support CTCA close to working properly but the protocol standard RSCS uses across the CTCA is not sufficient to keep things synchronised between RSCS and a TCPNJE device. BSC lines support ENABLE and DISABLE CCWs which allow additional information to be conveyed between the two. |
This could well be true and if so probably should be fixed. However, TCPNJE is based on COMMADPT and there is a typedef COMMADPT in the htypes.h I am looking at, not far above where typedef TCPNJE is located. |
The fact that COMMADPT is in htypes.h is a bug I need to fix! It shouldn't be there! I probably made this error 15+ years ago. |
Ivan, You are being too hard on yourself. I think if you remove typedef COMMADPT from htypes.h you will probably find that anything that includes hstructs.h but does not include commadpt.h (which is nearly everything) will fail to compile. To deal with this issue properly, struct DEVBLK should be redesigned to allow device type independent information and device type specific information to be properly separated. However, doing that would create all sorts of incompaibilities with everything else out there so this is another of these situations when it would be better to just let sleeping dogs lie rather than try to fix something that isn't really broken. |
Added a slightly modified version of the TCP/NJE support based on Pete's version. I just changed all reference to the TCPNJE typedef to "struct TCPNJE" and everything compiles just fine. tcpnje.c and tcpnje.h (and the necessary changes to Makefile.am) where added to the current version (also added a .md version of the README.TCPNJE) |
Fish, would you be kind enough to integrate it with the Doze build?
If any problem arises please ping me! |
Ivan, I think the least you could do is to honour the request I made in my first response to this issue, that you remove any indications that I might provide support for this arrangement from the code and documentation. I particularly would like the email address I included removed. It would be even better, if you could remove my name altogether. |
10-4.
10-4! |
FYI to Ivan (and others): Main README document: (which can be reached by scrolling down to the bottom of our main repository web page at https://github.com/SDL-Hercules-390/hyperion). Notice that there's a "Adding New Files to Hercules" link in the "HERCULES INTERNAL READMEs" section which documents what needs to be done: You did the *Nix side just fine! You just did't do the Windows (MSVC) stuff, that's all. But I'll take care of it for you this time. The changes needed for Windows basically involve updating the MSVC makefile fragments (e.g. OBJ_CODE.msvc, MODULES.msvc and MOD_RULES2.msvc) and the various Visual Studio files (e.g. Hercules_VSnnnn.vcxproj, Hercules_VSnnnn.vcxproj.filters, etc). That's about it. But don't worry about it. I'll take care of that for you this time. I just wanted you (and others) to be aware that there is a documented procedure that explains what needs to be done for the Windows side of things. I created so that you guys can do it yourself without me having to always do it, just in case anything should happen to me. But as I said, don't worry about it this time. I'll take care of it. I just wanted you to be aware of the documented procedure... ...for next time! I'll go ahead and add a new entry for your new TCPNJE readme to the "Features and Operation" section of the main README too, which it looks like you also forgot to do. But it's no big deal. I'll do it. |
I'll take care of it. At least in the README anyway. I haven't looked at actual source code files yet. Is there anything there I need to get rid of? |
How so? Item 4 of the license clearly states that "machine-executable forms of modified versions of the Software" may be distributed (which is what I presume you mean by "release", i.e. "release" = "machine-executable form").
Quite true. And your point regarding this is . . . . . . . . . . . . . . . . . . . what?
Sure it is! Item 4! I see absolutely no license violation whatsoever. None. Nada. |
Ivan (@ivan-w), Fish wrote:
Have you had a chance to test my patch yet? It seems to work fine for me, but I wanted you to give it your approval before committing it. Or should I go ahead and commit it without your testing it? Or is there another way you want to do it? (such as not doing it at all?) Let me know! Thanks. |
DNBFS:
And there are a slew of those guys, so it's probably trivial. |
DNBFS? What the heck does that mean? (Sheesh! gcc/clang is such a royal PITA! It compiles fine on Windows!) |
(FYI: I was specifically trying to prevent modifying any of Peter's code! But thanks to gcc/clang it looks like we might have no choice!) |
DNBFS stands for: Does Not Build From Source |
I don't see a complete correspondance between a release and a machine-executable form. I would argue that a machine-executable form would likely not include source and documentation whille a release may include these things. Anyway, whether a release involves a machine-executable form or not, I believe the key thing is that the license requires that source is provided in all cases. My point is that if the source is provided in some sort of archive format in the release, a zip file for example which contains just the source of the modified form of the Software, this does not meet the requirements of the license in item 3 of the granted rights which requires the Software and modifications to it to be separate. In the specific case of machine-executable forms of modified versions of the Software, restriction c to granted rights item 4 is very clear where it says "You must ensure that all modifications included in the machine-executable forms are available under the terms of this license." |
Source, both original and modified, is available to anyone at any time for free via our git source code repository.
And they are. What leads you to believe otherwise?
They are separate. I honesty don't understand your complaint at all, Peter. I'm very sorry, but you're making no sense to me whatsoever. You appear to me to be trying desperately to grasp at what are essentially non-existent straws. I mean, I'd like very much to make you happy, but I'm having trouble understanding what it is that you're unhappy about or what you feel we should do to make you happy. Does anyone else know? |
I can readily see the modified source in the source code repository as it is right now. How do I get to see the source that corresponds to a particular release (not that there is any requirement for this to be available)? More pertiently, how do I get to see the original unmodified source and how do I get to see the modifications made to it for a particular release? Perhaps this is obvious to more seasoned github users and I am insufficiently well versed in the details of how to use this medium, in which case please forgive my ignorance and point me to the documentation where it says how to access these things. (Aside from any license requirement, I would greatly appreciate being able to view a searchable change log which includes a meaningful description of what changes were made and when. It would make it so much easier to track down when things went awry. As far as I know, there used to be one but it was taken away in favour of a link to the git repository which I find to be a complete nightmare to search for particular modifications, unless of course I am failing to understand how to do it properly?)
That the terms of the license state that the modifications must be separate from the Software and I can't see how they are separate. (For clarity, I am of course referring to the modifications to the source here, not any modifications to the machine-executable form which neither me nor I believe the license expects anyone to be able to see directly.) Maybe the answer is right in front of me and I just don't know how to work this github thing well enough?
Forgive me if I am failing to understand something obvious here but I don't follow how a zip file containing modified source code and no patches (or other forms of descriptions of modifications that I can see) can be said to contain separate Software and modifications?
I didn't want to get sidetracked into this license issue but Ivan brought it up and I responded with my thoughts on the matter. Then you weighed in and disagreed with me which is fair enough but this has allowed Ivan to duck out of responding to my further comments in the same posting (the one ending in "How?").
One thing that is making me unhappy is that Ivan has dropped out of the conversation I was having with him. Ivan, can you please respond to the points I made in my posting much further back that ends in "How?"? While you are at it, could you also say something in response to what I said in my very first posting in this thread about the email I received about two months ago stating I was in some sort of discussions with you and Fish regarding this topic, discussions which I don't recall actually happening? |
Ivan (@ivan-w),
Ah. I never heard that before. Regardless, please try the below slightly revised version of my patch instead: It now builds (and runs) just fine for me on my CentOS 6.10 VMware test system. (My apologies for not having tested my original patch there before asking you to test it! I guess I got busy and forgot!) If it builds and runs okay for you this time (it should!) then I'll go ahead and commit it and then close this issue. But please respond to Peter's concerns too! (please?) He specifically mentioned his concern about your having dropped out of the conversation / discussion after my having responded to him. I'm not sure what (if anything) we need/should/can do to make him happy, but as I'm sure you know (and agree with too), he's such an important long time contributor to the Hercules project that IMO we should at least try to meet any reasonable demand of his to in order to make him happy/comfortable with what we're doing with his code/contribution to Hercules (which he doesn't seem to be right now which is causing me considerable concern). As for me, I believe I've done all that I can. Maybe you (or someone else?) can do more? Thanks. |
At this point I'm thinking that I'm going to start from scratch. In the process I'm also going to do a "dummy" device driver that can be used as a skeleton for future device handler developers as well as an I/O exerciser. |
Sounds good to me! |
Hi Ivan Given that I currently have put the TK4- release process on hold waiting for #289 to be fixed, I intend to use the wait time to integrate Bob's NJE38 product into the system. Thus the shy question: Where do you stand with the TCPNJE port? Cheers |
Hi Jurgen and team
I’ve put the fix in for the select CPU burn issue. I believe that is sufficient to fix the reported issue. I’m working through the logger thread stuff now. Oddly keep the logger thread alive for longer during shutdown removes a lot of complexity. It’s going to be a few days before I’m happy with proposing some further changes.
Andy
…Sent from my phone
On 29 Apr 2020, at 12:47, Jürgen Winkelmann ***@***.***> wrote:
Hi Ivan
Given that I currently have put the TK4- release process on hold waiting for #289 to be fixed, I intend to use the wait time to integrate Bob's NJE38 product into the system. Thus the shy question: Where do you stand with the TCPNJE port?
Cheers
Jürgen
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Andrew Gadsby (@agadsby) wrote:
The fix is in your personal fork of SDL Hyperion, yes. I can see that from the helpful GitHub mention in issue #289. But your fix is still not in the official SDL Hyperion repository yet. It's only in your repository (fork), but not in our repository. I've never done it myself so I'm not one to speak, but I believe there's a way to submit/create a "pull request" requesting that we (the SDL Hyperion repository) please do a "pull" from your repository (fork) of the commit that you made that fixed the problem. Then we could Merge your pull request to get your fix into our repository and you'd get credit for it. OR.... The much faster and easier way (easier for both you and me) would be, since the fix is so simple, for me to simply directly commit your fix to our repository myself (but with you as the "Author" of the fix of course, so that you get proper credit for it). So I think that's what I'm going to do. Give me a few minutes and I'll commit your fix and close this issue. |
Thank you that’s a great plan. Once I have the logger changes I’ll try and do it properly on a new issue.
Andy
…Sent from my phone
On 29 Apr 2020, at 20:41, Fish-Git ***@***.***> wrote:
Andrew Gadsby ***@***.***) wrote:
Hi Jurgen and team.
I’ve put the fix in for the select CPU burn issue. I believe that is sufficient to fix the reported issue.
The fix is in your personal fork of SDL Hyperion, yes. I can see that from the helpful GitHub mention in issue #289. But your fix is still not in the official SDL Hyperion repository yet. It's only in your repository (fork), but not in our repository. :)
I've never done it myself so I'm not one to speak, but I believe there's a way to submit/create a "pull request" requesting that we (the SDL Hyperion repository) please do a "pull" from your repository (fork) of the commit that you made that fixed the problem. Then we could Merge your pull request to get your fix into our repository and you'd get credit for it.
OR.... The much faster and easier way (easier for both you and me) would be, since the fix is so simple, for me to simply directly commit your fix to our repository myself (but with you as the "Author" of the fix of course, so that you get proper credit for it).
So I think that's what I'm going to do. ;-)
Give me a few minutes and I'll commit your fix and close this issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
(Oops!) Wrong issue! Previous comment copied to Issue #289 where it belongs. Sorry about that folks! |
Hi Fish It's not that easy!
So, I will not hurry at the moment and go after the MVS side of implementing NJE38 in TK4- for now... Cheers |
Jürgen wrote:
Understood! That's why I said "at your earliest convenience". That means: "whenever you feel it's appropriate to do so". There's no rush! I was just letting you know that one of your stated road blocks had been removed and so you were now free to proceed. But if you want to wait until Andy has a more complete/better fix to the message pipe problem before releasing your code, that's fine! As I said, no rush. It's your call. I'm fine with whatever you decide. |
Hi Fish and Jurgen
I think I now understand what is causing the other issues, the pipe is blocking under load which will cause mayhem for the calling thread. I have a fix in my head for this which I will test out and then upload to a new issue early next week. I will also include my general logger cleanup as well.
Andy
… But if you want to wait until Andy has a more complete/better fix to the message pipe problem before releasing your code, that's fine! As I said, no rush. It's your call. I'm fine with whatever you decide.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Andrew Gadsby (@agadsby) wrote:
Wrong thread. (i.e. Wrong GitHub Issue.) THIS issue is #280, and has to do with Porting TCPNJE support from spinhawk to SDL Hyperion. The issue you should be posting your comments to is issue #289, the Panel issue under Linux. Also, it would be greatly appreciated if you would please NOT reply to GitHub Issues via email. Rather, it would be greatly appreciated if you would please instead logon to GitHub and reply directly from GitHub itself: ---> https://github.com/SDL-Hercules-390/hyperion/issues. (Don't worry that issue #289 is closed. That's okay. You can still post comments to closed issues) SUMMARY:
Thanks! |
Fish wrote:
Hi Jürgen! (@Juergen-Git) What's the current status of this issue? (Just asking!) AFAIK, the fix that was committed to close #289 **is** Andy's "complete solution". Yes? Thanks. |
Hi Fish from my point of view, this one can be closed, 208e766 is stable for many weeks now. Cheers |
Closing. |
Port Peter Coghlan's spinhawk TCPNJE 1.0 version from spinhawk to the current SDL version
The text was updated successfully, but these errors were encountered: