ndbc-csv#190
Conversation
nickgaray
left a comment
nickgaray
left a comment
There was a problem hiding this comment.
Minor comments in the review, but one overall comment is to add the file header to all source files
| osgi { | ||
| manifest { | ||
| attributes('Bundle-Vendor': 'Botts Innovative Research Inc.') | ||
| attributes('Bundle-Activator': 'org.sensorhub.impl.ndbc.Activator') |
There was a problem hiding this comment.
Bundle Activator attribute value must follow the package structure of the Activator file,
org.sensorhub.impl.sensor.ndbc
There was a problem hiding this comment.
Will be corrected in next push
| @@ -0,0 +1 @@ | |||
| org.sensorhub.impl.ndbc.NDBCDescriptor | |||
There was a problem hiding this comment.
Like the activator, the package name must follow the same structure:
org.sensorhub.impl.sensor.ndbc
There was a problem hiding this comment.
Will be corrected in next push
| setDoubleValue(dataBlock, index++, rec.lon); | ||
| setDoubleValue(dataBlock, index++, rec.windSpeed); | ||
| if(rec.windDir == null) | ||
| dataBlock.setDoubleValue(index, Double.NaN); |
There was a problem hiding this comment.
index here should also be increased by 1
There was a problem hiding this comment.
Will be corrected in next push
Is there a standard header we are using now? Still the Mozilla pubic license? |
|
Think I have a better handle on dealing with SSL certs in the context of the osh-node project. This driver uses https when polling the ndbc server. So I was getting SSL handshake errors running from launch.sh initially. I added the ndbc cert file to 'osh-node/dist/common/trusted_certificates' and it works. Not sure where it should be located in the driver- resources maybe? At any rate, I've pushed latest code up to the repo. I think I covered all the changes in Nick's comments. The root build.gradle file required some minor changes also to get my local build to work. I will ask tomorrow morning about best route for adding crt file and resolving the build file issue in the osh-node repo. |
|
|
||
| // #STN LAT LON YYYY MM DD hh mm WDIR WSPD GST WVHT DPD APD MWD PRES PTDY ATMP WTMP DEWP VIS TIDE | ||
| public static boolean checkHeader(String header) { | ||
| // TODO check fields are as expected |
There was a problem hiding this comment.
If this is a planned enhancement to the module, make sure we log it in the backlog with enough detail to come in here and complete.
| // #text deg deg yr mo day hr mn degT m/s m/s m sec sec degT hPa hPa degC degC degC nmi ft | ||
| public static boolean checkUnits(String units) { | ||
| String [] fields = units.split("\\s+"); | ||
| // TODO check units are as expected |
There was a problem hiding this comment.
If this is a planned enhancement to the module, make sure we log it in the backlog with enough detail to come in here and complete.
| try { | ||
| return Double.parseDouble(s); | ||
| } catch (NumberFormatException e) { | ||
| logger.error("", e); |
| try { | ||
| return Integer.parseInt(s); | ||
| } catch (NumberFormatException e) { | ||
| logger.error("", e); |
|
|
||
| public class BuoyEnums { | ||
|
|
||
| public enum ObsParam |
There was a problem hiding this comment.
Do we need to encapsulate this enum within the parent class or will it suffice to just remove the inner enum and change the outer class to an enum?
3 participants
Main change here is to support CSV file reading from NDBC data access site. I also simplified the output structure so that there is only one output now. I may go back in and mod that to more closely model what the buoys provide- not all buoys support all observables.
I blew away a lot of the old code so diff may not help much. Just let me know if it's confusing and I can walk thru a little more with you.