SuperDARN / rst

Radar Software Toolkit (RST)
https://superdarn.github.io/rst/
GNU General Public License v3.0
22 stars 18 forks source link

C90 vs. C99? #289

Closed mts299 closed 4 years ago

mts299 commented 4 years ago

As I am tirelessly working on the elevation code, I am noticing we are still using C90 for compiling our code. Is there any reason?

C90 is causing more compiler warnings because people are using C99 syntax/types that are not supported in C90. Also, C99 as fixed several bugs with C90.

So can we move to C99 or C11?

Here are some links supporting to moving to C99: https://stackoverflow.com/questions/41535927/compatibility-of-c89-c90-c99-and-c11 http://www.simplyembedded.org/archives/battle-of-the-standards-round1/

ksterne commented 4 years ago

This is a great question. I can't say I know enough about the quirks, differences, advantages, and disadvantages of each. So my response is not very much from a technical side of things. My instinct is that we should upgrade to a newer syntax/standard.

I don't know why we are still using C90 for compiling aside from some of the initial building blocks of this code might go back to the mid to late 90s and just nothing was updated. It might be worth investigating if there's a dependency (that we treat more like it's part of the RST) that needs C90 compiling.

This sounds like a major overhaul to a lot of our files as they might need updating for C99 or C11. I'd say that we should try for C11, but maybe that is too big of a jump. Maybe this isn't too bad as I'm now reading #293.

Great job on starting up a project related to this and other things we need to update.

egthomas commented 4 years ago

At least a dozen radars in the network are still using QNX4 (circa 1997) for their radar control software. If we ever want to be able to port these updates to the dmap, rawacf, fitacf, or other libraries to those radars then we need to make sure we retain backwards compatibility with those sites. That is after all one of the strengths of SuperDARN - common analysis software.

mts299 commented 4 years ago

Keeping the discussion here so that people can test and report potential issues with the code on the PR. Compiler changes do not change how the code analysis the data that is not how compilers work they focus on the updates to the programming language. As some changes to the language can break backward compatibility of compiling that is not the case with C99 - C11 as they were updates to the C90 code having bugs and tight restrictions that people did not enjoy programming in. So no backward compatibility will be harmed. If you have concerns @egthomas then please test it and get back to me.

The only issue I can actually see is if radars actually pull RST down onto their computers? My thought is that this is not the case since they use ROS and do no produce fitacf files at the site. Plus many radars moving are moving to newer systems since we cannot keep supporting QNX4/other hardware. why not move the software along too :p

egthomas commented 4 years ago

As I am tirelessly working on the elevation code, I am noticing we are still using C90 for compiling our code. Is there any reason?

You asked if there was any reason we are still using C90 and I provided one.

The only issue I can actually see is if radars actually pull RST down onto their computers? My thought is that this is not the case since they use ROS and do no produce fitacf files at the site.

Yes, there are fitacf-level data products being produced on-site at the radars (particularly real-time measurements). Much of the codebase is shared between the various iterations of ROS and RST.

Plus many radars moving are moving to newer systems since we cannot keep supporting QNX4/other hardware. why not move the software along too :p

Just because some groups are migrating to newer systems does not mean all of the other institutions operating radars in the network have the same resources to do so.

mts299 commented 4 years ago

K never mind.

asreimer commented 4 years ago

@egthomas I think some of these concerns are not the DAWG's problem. This is DAWG, in charge of RST. We are not in charge of ROS. I think it would be reasonable to work with whoever is in charge of ROS to help them port changes as needed, but isn't that fundamentally their job?

Yes, there are fitacf-level data products being produced on-site at the radars (particularly real-time measurements). Much of the codebase is shared between the various iterations of ROS and RST.

Not quite. There's a bunch of "RST-like" stuff in ROS, but not the other way around. Also, there's several different versions for the different flavours of SuperDARN radar systems out there. For example, at least one version of ROS fitacf is basically fitacf2.0. It hasn't been updated in almost 2 decades. No one has ported any of the recent changes to fitacf2.5 into ROS (at least the version I'm familiar with).

Just because some groups are migrating to newer systems does not mean all of the other institutions operating radars in the network have the same resources to do so.

So, do they have resources to maintain ROS at all? Why do we need to maintain backwards compatibility if ROS isn't even pulling in backwards compatible versions of fitacf2.5 from previous (pre-github) versions of RST?

This is screaming out of scope for DAWG.

kkotyk commented 4 years ago

I agree, this codebase needs a drastic update to modernity. No radar group is running the same code any more and ROS only uses very small parts of RST anyway. It should be up to groups out there to make RST code work with their radar if a new update is needed, although I doubt many people are making updates to ROS based systems anyway. From personal experience, I think people are playing with fire making changes to ROS at this point.

mts299 commented 4 years ago

Hi everyone,

Thank you for bringing this issue back to light. I would like to keep moving RST forward to meet better programming practice as there are there for a reason. I would even want to look at how much do we need to be "backward compatible," as this can be the destruction of most software libraries. Please DO NOT reply to this question, just food for thought and way out of scope. I would rather discuss that in a teleconference.

Moving on, @egthomas I am sorry, I don't know ROS-RST radar system, and I don't know everything there is to know about superDARN things. I can see the concerns, but also some collaboration would be more than welcome. I am not here to argue why we should move to C99; I already did that with links. I am here to get the groundwork for what is possible in moving just one-step forward. As I asked the engineers at superDARN Canada, everyone mentioned we do not use RST with our ROS radars, so this made me believe this might be the case with most radars. Maybe if you know specific radars, this would help us figure out better solutions.

I think @asreimer and @kkotyk nailed the real issue on the head. What is the scope of RST? Are we in charge of ROS code too? My understanding is we deal with fitacf, and higher so this makes me believe we are more for the users than the radar operations. There was even a PR #193 that was closed because people didn't want to bring ROS code back into RST. Something to keep in mind?

Anyways, I am not here to argue or plays devil's advocate. If you are, please find another PR because I will stop helping as a hard communication boundary of mine. I want collaboration.

Thank you again for the comments, I think we will need more insight from @ecbland and @ksterne about scope and direction.

ecbland commented 4 years ago

Hi All. I will put this issue on the agenda for the DAWG telecon on 1 June, and I hope we can find a good solution. We would welcome some additional input (e.g. from PIs) either before or during the meeting.

ksterne commented 4 years ago

To come back to the discussion on do we/don't we, @ecbland and I are in agreement that we'll move ahead with this. Thank you for the couple of people that have voiced concerns. We've identified some plans and actions that we'll take (even some that are a little outside of the scope of the DAWG) should these problems come up. We'll communicate this to PIs in September.

ksterne commented 4 years ago

@mts299, with #293 merged in do you think this issue can be closed?

mts299 commented 4 years ago

Yup! just got back from holidays.