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

Initial changes and adaptations for embedded use #7

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

Conversation

emaayan
Copy link

@emaayan emaayan commented Feb 11, 2023

  1. added library.json as manifest
  2. added small type casting due variable size being truncated in arduino
  3. replaced some ' chars which won't display well on LCD
  4. replaced types in zmanim that linter didn't like

Copy link
Owner

@yparitcher yparitcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to have arduino specific files in the repo.
The library is designed with a 32 bit int (most modern platforms). to adapt it to a 16 bit int would need to change many of the signatures to guarantee correctness, not just the few you changed

Comment on lines +14 to +15
,"-I windows"
,"-D _WIN32"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no need to define windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one of the hardest things i had here, i wouldn't call it defining windows as more like for platorms who don't have stpncpy , if i won't include the stpncpy.h and the source the file the avr-gcc will fail cause it won't find them, i don't know of any other way to handle this.

Comment on lines -23 to +24
const char* hchar[]={ "׆", "א", "ב", "ג", "ד", "ה", "ו", "ז", "ח", "ט", "י", "כ", "ל", "מ", "נ", "ס", "ע", "פ", "צ", "ק", "ר", "ש", "ת", "״", "׳"};
const char* hmonth[]={ "אדר א׳", "ניסן", "אייר", "סיון", "תמוז", "אב", "אלול", "תשרי", "חשון", "כסלו", "טבת", "שבט", "אדר", "אדר ב׳"};
const char* hchar[]={ "׆", "א", "ב", "ג", "ד", "ה", "ו", "ז", "ח", "ט", "י", "כ", "ל", "מ", "נ", "ס", "ע", "פ", "צ", "ק", "ר", "ש", "ת", "\"", "'"};
const char* hmonth[]={ "אדר א'", "ניסן", "אייר", "סיון", "תמוז", "אב", "אלול", "תשרי", "חשון", "כסלו", "טבת", "שבט", "אדר", "אדר ב'"};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the geresh & gershaim better,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer not to have arduino specific files in the repo. The library is designed with a 32 bit int (most modern platforms). to adapt it to a 16 bit int would need to change many of the signatures to guarantee correctness, not just the few you changed

so far it has worked splendidly with what i have right now.
currently arduino doesn't have memory to use the zmanim.c, so i plan to port it to esp32 which i believe is a 32bit library.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the geresh & gershaim better,

unfortuently those don't display well, in LCD, the alternative the way i see it is to have a completely different set of strings just for embedded, but then you'd have to maintain 2 sets.

@emaayan
Copy link
Author

emaayan commented Feb 13, 2023

I would prefer not to have arduino specific files in the repo. The library is designed with a 32 bit int (most modern platforms). to adapt it to a 16 bit int would need to change many of the signatures to guarantee correctness, not just the few you changed

if you mean library.json, it's not exactly arduino specific, it's manifest for library depdencies, much like rockspec.
https://docs.platformio.org/en/latest/librarymanager/index.html#librarymanager

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.

2 participants