Skip to content
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

feat: ts rewrite and DPT 22, 28 29, 213, 235, 242, 249, 251, 275, 99, 60001 support #2

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

robertsLando
Copy link
Collaborator

@robertsLando robertsLando commented Apr 2, 2024

Improvements/Fixes:

  • Error On dpt2 multiple subtypes 001: 010 011 012
  • Fix logs on dptlib/index.js (scalar and range not defined)
  • Varius dpt typos
  • DPT237 apdu_data[1] and [2] assigned to an array instead of a number
  • Bumped all deps to latest and fixed all security issues
  • Bump min nodejs version to 16
  • Remove unused multi-tape dep
  • Add linter and prettier for better code style
  • Improved tests
  • dptlib is now created statically, this makes bundlers happier (no need to follow dinamic require)
  • Export of dptlib object. This is useful for applications in order to get informations about datapoints types and subtypes definitions
  • Connection.js has been renamed to KnxClient
  • Other typos/bugs/optimizations code wise
  • Support for DPT: 22, 28 29, 213, 235, 242, 249, 251, 275, 999, 60001.
  • Fixed DPT 9,10,18,20,21
  • Ensure dpts and other code classes use correct log instance to respect log level passed in options
  • Move version print in KnxClient constructor to prevent unwanted logs on console

BREAKING CHANGES

  • Minimum required nodejs version is now 16+ (that is already EOL btw)
  • Rename Connection to KnxClient and new operator is now manadatory

Fixed errors:

- On dpt2 multiple subtypes 001: 010 011 012
- Ensure every file uses knxlog and not log-driver directly
- Fix logs on dptlib/index.js (scalar and range not defined)
- Varius dpt typos
- DPT237 apdu_data[1] and [2] assigned to an array instead of a number
@robertsLando
Copy link
Collaborator Author

robertsLando commented Apr 2, 2024

TODOs:

  • Fix missing types
  • Fix workflow
  • Fix package.json
  • Rewrite tests using TS
  • Add eslint+prettier
  • Check logger

@ekarak
Copy link
Owner

ekarak commented Apr 2, 2024

please consider:

  • ensuring change to TypeScript won't break our dependents: https://www.npmjs.com/package/knx?activeTab=dependents
  • bumping up major version. We're basically porting the library to another language.
  • keeping a common text header to all files
  • (I'm sure I'm forgetting something)

@ch-zacmo
Copy link

ch-zacmo commented Apr 3, 2024

Nice to see that someone want to invest time in knx.
I wanted to contribute too since I use it in multiple small and big project. My goal was to add support for knx secure and some features to be able to import the groupAddress for direct DPT mapping, but I didn't have much time recently. Now if the project is going with TS, I will go in my own direction and stay vanilla. I think we need to weigh up whether TS will bring a lot to the table.

@robertsLando
Copy link
Collaborator Author

Now if the project is going with TS, I will go in my own direction and stay vanilla.

Could I ask you why?

@robertsLando
Copy link
Collaborator Author

robertsLando commented Apr 3, 2024

please consider:

  • ensuring change to TypeScript won't break our dependents: npmjs.com/package/knx?activeTab=dependents
  • bumping up major version. We're basically porting the library to another language.
  • keeping a common text header to all files
  • (I'm sure I'm forgetting something)

@ekarak Sure! I will let you know once I end with this but my idea is to keep it full back compatibile!

@ch-zacmo
Copy link

ch-zacmo commented Apr 3, 2024

Now if the project is going with TS, I will go in my own direction and stay vanilla.

Could I ask you why?

The benefit vs the work necessary to set up, undertand and maintain typescript project will complicate things for me and my colleague. I already don't have much time to do it in the simple way.

@robertsLando
Copy link
Collaborator Author

The benefit vs the work necessary to set up, undertand and maintain typescript project will complicate things for me and my colleague. I already don't have much time to do it in the simple way.

I can understand but trust me I had your same opinion about TS at start and now I feel so bad when I work with an untyped codebase. Also every time I do a TS refactor I found tons of issues and bugs caused by typos that js simply will never spot.

Could you wait a bit I end with this so I can help you working with the TS codebase? It would be much better and easier to debug...

@robertsLando robertsLando marked this pull request as ready for review April 3, 2024 13:03
@robertsLando
Copy link
Collaborator Author

robertsLando commented Apr 3, 2024

@ekarak the PR is almost done (I only need to add eslint and prettier) I kindly ask you if you can take a look at it and also run the wired tests on your end (I don't have a way to run them)

Now to run them simply use: npm run test:wired

@robertsLando
Copy link
Collaborator Author

robertsLando commented Apr 3, 2024

@ekarak Why no break on this line? Is this expexted? There is also a comment that doens't makes me feel safe adding it

knx/src/FSM.ts

Lines 921 to 922 in ccda6fe

AddCRI(datagram) // no break!
// eslint-disable-next-line no-fallthrough

@robertsLando
Copy link
Collaborator Author

robertsLando commented Apr 3, 2024

Ok I'm done here, please @ekarak make a review when possible :)

The only breaking changes I introduced are:

  • Minimum required nodejs version is now 16+ (that is already EOL btw)
  • The new Operator is now mandatory when creating a new Knx connection

@ch-zacmo
Copy link

ch-zacmo commented Apr 3, 2024

Wired test are ok for me. I just did the read storm with 4 as I don't have a bigger actuator.

@robertsLando
Copy link
Collaborator Author

@ch-zacmo Thanks for your feedback! 🙏🏼 Hope @ekarak tests will end well too

@robertsLando robertsLando changed the title feat: ts rewrite feat: ts rewrite and DPT 22, 28 29, 213, 235, 242, 249, 251, 275, 99, 60001 support Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants