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

#1131 First try to implement storing keyer memories into serial EEPROM #1435

Merged
merged 2 commits into from
Mar 11, 2018

Conversation

phaethon
Copy link
Contributor

This is work in progress. It currently works only for the first memory, but I haven't found the bug yet. May be I am misunderstanding how SerialEEPROM_24Cxx_WriteBulk should be called?

Serial EEPROM is mandatory for this f-ty.

Please, comment on the value of EEPROM_KEYER_MEMORY_ADDRESS
I am not checking the size of EEPROM as the minimum supported size is enough.

@db4ple
Copy link
Collaborator

db4ple commented Feb 10, 2018

Hi Andreas, Eriks,
I propose not to merge this now. Let us have a look at it and discuss a little how we are going to use the EEPROM in general before we integrate this.

@Eriks: I will have a look at your code this weekend and see if I find your problem with storing data.

@phaethon
Copy link
Contributor Author

I am fine with having discussion before merging as I agree that some overall plan for EEPROM usage would be good. Are there any other ideas what to store in EEPROM besides configuration variables, keyer memories, and #236?
One more idea is simple logging. Since last week I have RTC, may be at some point of time I could look into that.
For voice memories currently recommended sizes are too small.

@df8oe
Copy link
Owner

df8oe commented Feb 10, 2018

Hi Eriks,

storing of SSTV images, voice, logging and so on in is planned for SDcard. But this is not mcHF related - only OVI40. Keyer memories CAN be stored there - but it will be very wise to discuss the structure of the storage.

@db4ple
Copy link
Collaborator

db4ple commented Feb 13, 2018

Hi,
In order to combine flexibility with efficient use of available storage, I suggest to consider a block-wise chained memory allocation scheme with flexible data length. I.e. a fixed size block consists of a header (id, len) and storage space of a fixed size. A storage space can span multiple blocks. Think of it as a very simple file system, where each id represents a "file".
We could use this to store arbitrary large data structures such as strings, memories, etc. Different from a file system I would suggest that once a "file" has been written for the first time, it is not allowed to grow, only to shrink or to be deleted.
This makes management relatively easy.

@phaethon
Copy link
Contributor Author

Essentially this is implementing malloc. Do we really need it right now? It might make sense for dynamically allocating/freeing memory for received SSTV images, but for configuration information, which is static, it should be sufficient just deciding on the address and consider it reserved for other uses.
I am just afraid such significant complication as it would postpone this PR, which solves something that people could use right away. Then we can later refactor this if needed.

@df8oe
Copy link
Owner

df8oe commented Feb 16, 2018

Hi Eriks,

we are not lucky to burn EEPROM cells with structures which can (and will) get problems if structures change later. There are already wishes for storing more informations to band specific storage places, and there will come up frequency memories which will need all the storage of a normal band specific storage place. I feel that a "data structure" like Danilo proposes will work for all extensions that will come up the next future.

Additinally we should pay attention for maximum capacity of virtual EEPROM. There will be overflow in the future, so no backup possibility from this point on.

And the last, most important thing: first all memories should work not only one.

Let us discuss about a file system like data block system and define the first identifiers.

@DG9BFC
Copy link

DG9BFC commented Mar 2, 2018

just an idea ... memory for keyer should also be usable in other modes ... and editable with keyboard
so then we have "macro" like texts that can be used in cw or rtty or psk (or any othere mode in future)
i would guess that the keyer (or better say the memory of to be send text) could be useful in any mode
(cq cq de callsign pse k can be used also in psk mode or rtty as an example)

@phaethon
Copy link
Contributor Author

phaethon commented Mar 6, 2018

@DG9BFC Memory keyer is usable for playing recorded message in PSK from the start.

@DG9BFC
Copy link

DG9BFC commented Mar 6, 2018

ok on that ... will the memory texts be available in ALL modes (rtty included)? then they also can be used in whatever mode will come ... and that was the idea behind my suggestion (available in all modes) ... GOOD WORK, WELL DONE

@db4ple
Copy link
Collaborator

db4ple commented Mar 10, 2018

@phaethon: I don't see that we get any progress on the general eeprom storage code. I suggest to change the start address to 0x1000h . Minimum accept EEPROM size is 32k, so 4k boundary is okay, I'd say.

@phaethon
Copy link
Contributor Author

Changed memory start address to 0x1000. Now it works for all 3 memories.
May be we can reserve part of the memory for "static use" (like in my code), and other part for advanced memory management (to be implemented later on)? As I wrote earlier I don't see the immediate benefit of advanced mechanism right now (at least not yet and not for such use cases where fixed amount of memory is needed as for this PR).

@db4ple
Copy link
Collaborator

db4ple commented Mar 11, 2018

Hi Eriks,
unfortunately it seems to me you did not use git rebase as described in the contribution guide. You probably used git pull from your github repository, and because of that your pull request now contains 40 changed files coming from 56 commits. We will not merge the PR in this form. Sorry.
But I think you can quickly resolve that by running a git pull ... followed by git push -f ... (please see instructions for details). If you need help, let me know.

Thank you!

EDIT: To be honest: Maybe these extra commits will go away once merged by Andreas, DF8OE via git, but we won't that risk here.

@phaethon
Copy link
Contributor Author

@db4ple please, see now.

@db4ple
Copy link
Collaborator

db4ple commented Mar 11, 2018

Thanks Eriks, result looks perfect.

Just for my curiosity did it work more or less with the approach I suggested?

@phaethon
Copy link
Contributor Author

Roughly, yes. In addition, I did git reset ...

@df8oe df8oe merged commit ace8850 into df8oe:active-devel Mar 11, 2018
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.

4 participants